update in-tree pyasn1 and pyasn1-modules

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

unspecified
mozilla59
Points:
---

Firefox Tracking Flags

(firefox58 fixed, firefox59 fixed)

Details

(Whiteboard: [psm-assigned])

Attachments

(7 attachments)

The in-tree versions of pyasn1 and pyasn1-modules are a year or two old. The newest versions have many improvements.
Comment hidden (mozreview-request)
Assignee: nobody → dkeeler
Priority: P2 → P1
Whiteboard: [psm-backlog] → [psm-assigned]

Comment 7

2 years ago
mozreview-review
Comment on attachment 8926632 [details]
bug 1413336 - (1/7) update pyasn1 to 0.3.7

https://reviewboard.mozilla.org/r/197858/#review203538
Attachment #8926632 - Flags: review?(ted) → review+

Comment 8

2 years ago
mozreview-review
Comment on attachment 8926633 [details]
bug 1413336 - (2/7) update pyasn1-modules to 0.1.5

https://reviewboard.mozilla.org/r/197860/#review203540
Attachment #8926633 - Flags: review?(ted) → review+

Comment 9

2 years ago
mozreview-review
Comment on attachment 8926634 [details]
bug 1413336 - (3/7) fix pycert.py and pykey.py with respect to pyasn1/pyasn1-modules updates

https://reviewboard.mozilla.org/r/197862/#review204502

r+ with issues addressed and my question answered.

::: security/manager/ssl/tests/unit/pycert.py:84
(Diff revision 1)
>  feature value (see rfc7633 for more information).
>  
>  If a serial number is not explicitly specified, it is automatically
>  generated based on the contents of the certificate.
>  """
>  

We should probably update security/manager/ssl/tests/unit/requirements.txt as well in this commit or another.

::: security/manager/ssl/tests/unit/pycert.py:222
(Diff revision 1)
>  
>  
>  def getASN1Tag(asn1Type):
>      """Helper function for returning the base tag value of a given
>      type from the pyasn1 package"""
> -    return asn1Type.baseTagSet.getBaseTag().asTuple()[2]
> +    return asn1Type.tagSet.getBaseTag().tagId

Optional: Consider switching from the old `getBaseTag()` to the new `baseTag` property while you're here.

::: security/manager/ssl/tests/unit/pycert.py:516
(Diff revision 1)
>              # the OID for id_kp_serverAuth is incorrect in the
>              # pyasn1-modules implementation
>              return univ.ObjectIdentifier('1.3.6.1.5.5.7.3.1')

It looks like pyasn1-modules 0.0.6 fixed the OID, so we can remove this workaround.

::: security/manager/ssl/tests/unit/pykey.py:539
(Diff revision 1)
> -        algorithmIdentifier.setComponentByName('parameters', univ.Null())
> +        nullEncapsulated = univ.Any(encoder.encode(univ.Null()))
> +        algorithmIdentifier.setComponentByName('parameters', nullEncapsulated)

Hmm, I don't quite understand this change. Could you enlighten me?
Attachment #8926634 - Flags: review?(cykesiopka.bmo) → review+

Comment 10

2 years ago
mozreview-review
Comment on attachment 8926635 [details]
bug 1413336 - (4/7) make certificate serial number generation not depend on pyasn1 object string representation

https://reviewboard.mozilla.org/r/197864/#review204506
Attachment #8926635 - Flags: review?(cykesiopka.bmo) → review+

Comment 11

2 years ago
mozreview-review
Comment on attachment 8926636 [details]
bug 1413336 - (5/7) ensure text files generated by pycert et. al. have trailing newlines

https://reviewboard.mozilla.org/r/197866/#review204508

Heh, the lack of a trailing newline has always bothered me, but didn't seem worth fixing by itself.
Thanks.
Attachment #8926636 - Flags: review?(cykesiopka.bmo) → review+

Comment 12

2 years ago
mozreview-review
Comment on attachment 8926637 [details]
bug 1413336 - (7/7) regenerate all the certificates!

https://reviewboard.mozilla.org/r/197868/#review204510

rs=me.
Attachment #8926637 - Flags: review?(cykesiopka.bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8926634 [details]
bug 1413336 - (3/7) fix pycert.py and pykey.py with respect to pyasn1/pyasn1-modules updates

https://reviewboard.mozilla.org/r/197862/#review204584

::: security/manager/ssl/tests/unit/pycert.py:84
(Diff revision 1)
>  feature value (see rfc7633 for more information).
>  
>  If a serial number is not explicitly specified, it is automatically
>  generated based on the contents of the certificate.
>  """
>  

Good idea.

::: security/manager/ssl/tests/unit/pycert.py:222
(Diff revision 1)
>  
>  
>  def getASN1Tag(asn1Type):
>      """Helper function for returning the base tag value of a given
>      type from the pyasn1 package"""
> -    return asn1Type.baseTagSet.getBaseTag().asTuple()[2]
> +    return asn1Type.tagSet.getBaseTag().tagId

Yeah, good call.

::: security/manager/ssl/tests/unit/pycert.py:516
(Diff revision 1)
>              # the OID for id_kp_serverAuth is incorrect in the
>              # pyasn1-modules implementation
>              return univ.ObjectIdentifier('1.3.6.1.5.5.7.3.1')

Good catch.

::: security/manager/ssl/tests/unit/pykey.py:539
(Diff revision 1)
> -        algorithmIdentifier.setComponentByName('parameters', univ.Null())
> +        nullEncapsulated = univ.Any(encoder.encode(univ.Null()))
> +        algorithmIdentifier.setComponentByName('parameters', nullEncapsulated)

There's something wrong with the way univ.Any interacts with univ.Null right now. I think this should be possible without the encapsulation, but at the moment that just creates an AlgorithmIdentifier that's missing the Null tag. This essentially explicitly sets the value of the Any tag to 05 00 (i.e. NULL). (And it's a good thing you mentioned this because I hadn't noticed we've got the same bug in stringToAlgorithmIdentifiers.)
Thanks for the reviews! I decided to fix the not including NULL parameters for RSA algorithms - would you mind having a look at that changeset as well?
Er... actually that changeset is more about using the [] syntax over setComponentByName, but I also did fix the NULL issue.

Comment 23

2 years ago
mozreview-review
Comment on attachment 8928323 [details]
bug 1413336 - (6/7) replace setComponentByName with direct property setters

https://reviewboard.mozilla.org/r/199528/#review205482

Nice!
Attachment #8928323 - Flags: review?(cykesiopka.bmo) → review+

Comment 24

2 years ago
mozreview-review
Comment on attachment 8926634 [details]
bug 1413336 - (3/7) fix pycert.py and pykey.py with respect to pyasn1/pyasn1-modules updates

https://reviewboard.mozilla.org/r/197862/#review205484

Looks good, but unless I misunderstand, it looks like pycms.py needs updating as well. r=me with that.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to :Cykesiopka from comment #24)
> Comment on attachment 8926634 [details]
> bug 1413336 - (3/7) fix pycert.py and pykey.py with respect to
> pyasn1/pyasn1-modules updates
> 
> https://reviewboard.mozilla.org/r/197862/#review205484
> 
> Looks good, but unless I misunderstand, it looks like pycms.py needs
> updating as well. r=me with that.

Indeed! Good catch. Thanks for the reviews!
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=455e89a11733
Tree's closed right now, but I'll queue this up in autoland.

Comment 34

2 years ago
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/552ac666904c
(1/7) update pyasn1 to 0.3.7 r=ted
https://hg.mozilla.org/integration/autoland/rev/79d2717b12ea
(2/7) update pyasn1-modules to 0.1.5 r=ted
https://hg.mozilla.org/integration/autoland/rev/62a772c93461
(3/7) fix pycert.py and pykey.py with respect to pyasn1/pyasn1-modules updates r=Cykesiopka
https://hg.mozilla.org/integration/autoland/rev/37dd644dc984
(4/7) make certificate serial number generation not depend on pyasn1 object string representation r=Cykesiopka
https://hg.mozilla.org/integration/autoland/rev/d6c06eed38fe
(5/7) ensure text files generated by pycert et. al. have trailing newlines r=Cykesiopka
https://hg.mozilla.org/integration/autoland/rev/f9180211e874
(6/7) replace setComponentByName with direct property setters r=Cykesiopka
https://hg.mozilla.org/integration/autoland/rev/e972f1bffe69
(7/7) regenerate all the certificates! r=Cykesiopka
You need to log in before you can comment on or make changes to this bug.