Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Certificate.new vs Certificate.new(string) #519

Closed
no6v opened this issue Jun 21, 2022 · 4 comments
Closed

Certificate.new vs Certificate.new(string) #519

no6v opened this issue Jun 21, 2022 · 4 comments

Comments

@no6v
Copy link
Contributor

no6v commented Jun 21, 2022

require "openssl"

cert1 = OpenSSL::X509::Certificate.new
cert1.version = 2
cert1.serial = 1
cert1.subject = cert1.issuer = OpenSSL::X509::Name.new([["CN", "CN"]])
t = Time.now
cert1.not_before = t - 3600
cert1.not_after = t + 3600
cert1.public_key = key = OpenSSL::PKey::EC.generate("prime256v1")
cert1.sign(key, "sha256")

cert2 = OpenSSL::X509::Certificate.new(cert1.to_der)
p cert1 == cert2                      # => true

# cert1: from Certificate.new
p cert1.verify(key)                   # => true
cert1.serial = 2
p cert1.verify(key)                   # => false

# cert2: from Certificate.new(string)
p cert2.verify(key)                   # => true
cert2.serial = 2
p cert2.verify(key)                   # => true (this should be false)

I'm wondering this is why? Confirmed with:

  • RUBY_DESCRIPTION
    • ruby 3.2.0dev (2022-06-10T01:10:27Z master e75cb61d46) [x86_64-linux]
  • OpenSSL::VERSION
    • 3.0.0
  • OpenSSL::OPENSSL_VERSION
    • OpenSSL 3.1.0-dev
    • OpenSSL 1.1.1n 15 Mar 2022
    • LibreSSL 3.5.2
@no6v
Copy link
Contributor Author

no6v commented Jun 23, 2022

If I understand correctly, the cause of this is what is described here:
https://www.openssl.org/docs/man3.0/man3/i2d_X509.html

Any function which encodes a structure (i2d_TYPE(), i2d_TYPE() or
i2d_TYPE()) may return a stale encoding if the structure has been
modified after deserialization or previous serialization. This is
because some objects cache the encoding for efficiency reasons.

cert = OpenSSL::X509::Certificate.new(ARGF.read)
der, pem, text = cert.to_der, cert.to_pem, cert.to_text
cert.serial += 1
p der == cert.to_der   # => true
p pem == cert.to_pem   # => true
p text == cert.to_text # => false

This let me confused.

It would be nice to have a method to call i2d_re_X509_tbs().

https://www.openssl.org/docs/man3.0/man3/i2d_re_X509_tbs.html

If, after modification, the X509 object is re-signed with
X509_sign(), the encoding is automatically renewed. Otherwise, the
encoding of the TBSCertificate portion of the X509 can be manually
renewed by calling i2d_re_X509_tbs().

@rhenium
Copy link
Member

rhenium commented Jul 26, 2022

I agree with your analysis.

It's also confusing since the behavior is inconsistent between types, e.g., X509_REQ_set_version() updates the modified bit while X509_set_version() doesn't.

It would be nice to have a method to call i2d_re_X509_tbs().

Invalidating cache seems like a side effect, but I think we can add something like #tbs_to_der (just an example) to appropriate classes since it's a feature not present and is nice to have anyway.

@no6v
Copy link
Contributor Author

no6v commented Jul 28, 2022

I agree with your analysis.

Thank you.

It's also confusing since the behavior is inconsistent between types, e.g., X509_REQ_set_version() updates the modified bit while X509_set_version() doesn't.

Thanks for the information.

Invalidating cache seems like a side effect, but I think we can add something like #tbs_to_der (just an example) to appropriate classes since it's a feature not present and is nice to have anyway.

It sounds reasonable to me.

And for example, in the case of X509 Certificate, the TBSCertificate portion itself is one of a requisite information when those who want to manually validate SCT (Signed Certificate Timestamp) signatures embedded in ct_precert_scts extension.

Currently Certificate#extensions is immutable and Certificate#extensions= does not invalidate the cache, so it is a bit difficult to extract TBSCertificate portion other than that extension. If we have #tbs_to_der etc., it will be easy to do that.

segiddins added a commit to segiddins/openssl that referenced this issue Apr 30, 2024
Ref ruby#519

This makes verifying embedded certificate transparency signatures significantly easier, as otherwise the alternative was manipulating the ASN1 sequence, as in sigstore/sigstore-ruby@656d992
rhenium pushed a commit to segiddins/openssl that referenced this issue Jun 8, 2024
Ref ruby#519

This makes verifying embedded certificate transparency signatures
significantly easier, as otherwise the alternative was manipulating the
ASN1 sequence, as in
sigstore/sigstore-ruby@656d992
matzbot pushed a commit to ruby/ruby that referenced this issue Jun 8, 2024
Ref ruby/openssl#519

This makes verifying embedded certificate transparency signatures
significantly easier, as otherwise the alternative was manipulating the
ASN1 sequence, as in
sigstore/sigstore-ruby@656d992

ruby/openssl@99128bea5d
@rhenium
Copy link
Member

rhenium commented Jun 8, 2024

X509_REQ_set_version() updates the modified bit while X509_set_version() doesn't.

This has been fixed in OpenSSL (independently) in openssl/openssl#19271 which I think went to OpenSSL 3.2.0.

Also, #753 added Certificate#tbs_bytes as a wrapper around i2d_re_X509_tbs().

@rhenium rhenium closed this as completed Jun 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants