Skip to content

Conversation

@ovnicraft
Copy link

@ovnicraft ovnicraft commented Dec 15, 2019

TODO:

  • has a bug with public_bytes() in next lines fixed with fefd820
  • b64 for digest

David Arnold and others added 2 commits November 20, 2019 10:51
TODO: has a bug with public_bytes() in next lines
@ghost
Copy link

ghost commented Dec 15, 2019

There were the following issues with this Pull Request

  • Commit: 1005ede
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@ghost
Copy link

ghost commented Dec 15, 2019

There were the following issues with this Pull Request

  • Commit: 1005ede
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: fefd820
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@ghost
Copy link

ghost commented Dec 16, 2019

There were the following issues with this Pull Request

  • Commit: 1005ede
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: fefd820
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: f6db4d4
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

Copy link

@blaggacao blaggacao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some quick comments, I hope to be able to do a full review in the coming days. Honestly it might take well into the christmas holiday, though, I've to double check with @Deiber as well.

serial_element = XADES132.IssuerSerialV2(
# TODO implement, wtf?
DS.X09IssuerName(issuer_string(cert)),
DS.X509SerialNumber(f"cert.get_serial_number()")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite yet: <xsd:element name="IssuerSerialV2" type="xsd:base64Binary" minOccurs="0"/>
The whole ASN.1 object shall be DER encoded and put here as bytestring.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I compared the result from XML signed (xades-bes) and what get_serial_number() it's ok.
which schema file got it ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you implemented the legacy IssuerSerial

See the schema of IssuerSerialV2

Copy link

@blaggacao blaggacao Dec 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just saw our latest commit, the object itself is der encoded, not one of it's members...

usp := UnignedSignatureProperties, c. 4.3.6
udop := UnignedDataObjectProperties, c. 4.3.7
Electronic signature forms elements

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Electronic signature grammar (I guess that's what it is) by mode"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with that, just those ? look a little grammar-ish. But yep, in xml, that is probably not a common usage of a word...

udop := UnignedDataObjectProperties, c. 4.3.7
Electronic signature forms elements
Mention differences between nodes

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nodes -> modes

return XADES132.QualifyingProperties(*qp_elements, **qp_attributes)


class XAdESEPESSigner(XAdESSigner):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be an excellent idea, I have to think around it a bit.. Thanks 👍

@blaggacao
Copy link

blaggacao commented Dec 16, 2019

@ovnicraft Thanks a lot for your PR! I'll have a look asap, and meanwhile I should probably reconfigure the commitlinter 😉 - until then, let's ignore him.

@ghost
Copy link

ghost commented Dec 16, 2019

There were the following issues with this Pull Request

  • Commit: 1005ede
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: fefd820
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: f6db4d4
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: 57f3cd8
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@blaggacao
Copy link

@ovnicraft I thought a little about the class base implemnetaiton of the different signature modes. I like the idea, it's hackable. Not sure if you've seen it, I've put a (still) unused enum at the beginning of the file, it was intended to server as a type switch aprameter for a mdoe dispatcher, with your prob=osal that would be a class dispatcher based on the enum, what do you think?

Copy link

@blaggacao blaggacao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still a spec inconsistency about IssuerServialV2

Or am I missign something?

serial_element = XADES132.IssuerSerialV2(
# TODO implement, wtf?
DS.X09IssuerName(issuer_string(cert)),
DS.X509SerialNumber(b64encode(_der.encode_der_integer(cert.get_serial_number())))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec is something like:
XADES132.IssuerSerialV2(b64encode(_der.encode...(asn_obj)))

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO must defined from DS.X509SerialNumber element wil double check

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@ghost
Copy link

ghost commented Dec 18, 2019

There were the following issues with this Pull Request

  • Commit: 1005ede
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: fefd820
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: f6db4d4
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: 87eb612
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

Copy link

@blaggacao blaggacao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ASN object still is not the one required... :( - I feel you are close! 😉

serial_element = XADES132.IssuerSerialV2(
DS.X09IssuerName(issuer_string(cert)),
DS.X509SerialNumber(f"cert.get_serial_number()")
b64encode(_der.encode_der_integer(cert.get_serial_number()))
Copy link

@blaggacao blaggacao Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that's not the ASN object required here, it's the IssuerSerial object...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read this:
https://www.pyopenssl.org/en/stable/api/crypto.html#OpenSSL.crypto.X509.get_serial_number
I am calling get_serial_number()
Certificate has:

  • issuer
  • issuer serial number

Copy link

@blaggacao blaggacao Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see:

Returns: The serial number.
Return type: int

But what needs DER encoding is this RFC IssuerSerial ASN object

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

        IssuerSerial ::= SEQUENCE {
            issuer                   GeneralNames,
            serialNumber             CertificateSerialNumber
       }

taken from: https://tools.ietf.org/html/rfc5035

Copy link

@blaggacao blaggacao Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GeneralNames can be quite some different objects, but I haven't yet analyzed how that poses encoding challenges...

Copy link

@blaggacao blaggacao Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note also: it seems to be a sequence, or array of unnamed objects (if I understand the ASN spec grammar correctly).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO DER representation must be done at application (signxml) level; so pyopenssl and cryptography provide API to solve this. AFAIK i do it.

IssueSerial comes from certificate from pyopenssl in 2 objects

  • SerialName
  • SerialNumber

Here we need to identify what needs the V2 on new spec; in legacy they has 2 elements on XML and i understood in V2 just one: SerialNumber

You can check my PR X509Name class and my own implementation to reverse get_components()

In terms of X509 Object all Issue serial is represented by:

imagen

as RFC mentioned defined.

Copy link

@blaggacao blaggacao Dec 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not construe the mentioned ASN object using some ASN generator? (pyasn.1 - I guess) - and then DER encode through one of the APIs you mention.

Or could you point me to the pyopenssl IssuerSerial object? I haven't seen it. Is it a sequence as the spec mandates?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for the name string, what's the difference between your implementation and https://cryptography.io/en/latest/x509/reference/#cryptography.x509.Name.rfc4514_string ?

@ghost
Copy link

ghost commented Jun 6, 2020

There were the following issues with this Pull Request

  • Commit: fd15c36
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: 1005ede
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: fefd820
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: f6db4d4
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: 87eb612
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants