-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Java truststore support for PKCS#12 serialization #12393
base: main
Are you sure you want to change the base?
Conversation
Thanks for the PR, we’ll take a look tomorrow! |
Looking back on this again this morning, I noticed the contents of the java trust attribute weren't lining up with what OpenSSL had. Seems I missed accidentally embedded the attribute into itself instead of the Any Purpose attribute. The safebag attribute now looks identical to what OpenSSL is outputting. For a bit more clarity, this is how the attribute looks now:
|
And in both cases Java accepted it? That sort of lax structural parsing is worrisome. Does Java document the ASN.1 structure it nominally wants vs just empirically determining what it will accept? |
I agree with the concern here, as keytool does accept both. Unfortunately there isn't really any direct documentation on the topic, and I've instead been relying on the source for OpenJDK and OpenSSL- including OpenSSL's discussion on the topic (relevant comment). I spent some time looking through OpenJDK's source again, and while I see references to writing the usage OID, I don't see anything actually depending on it, which is puzzling. At this point my inclination is to align with what OpenSSL/OpenJDK do, as external tools may expect this behavior. As a quick list of things I've tried with both the correct and incorrect usage attribute:
At this point I have to assume the presence of |
@seanjmullan is there a spec for this? We'd like to land support for java trust store creation, but as @crbednarz has discovered it seems like Java is very lenient about the structure here and I want to be sure we don't create a future ecosystem compatibility issue. |
Sorry, just looking at this now. I'm finding it a little difficult to unpack what the main issues are. Can someone summarize for me and I'll have myself or someone on my team look into it more? Thanks! |
The core question is: What is the correct format for the JDK trust store attribute on a PKCS#12 bundle? (The question is provoked by it looking like there were two different structures that were allowed.) |
@crbednarz If you're up for it, I think the parts of this that are pure refactoring can be pulled into a separate PR, and then we can review/merge those while we figure out the exact attribute structure. |
0a382ae
to
b200336
Compare
b200336
to
d4fca2c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the new vector, please document it in test-vectors.rst
@@ -12,6 +14,9 @@ pub const SHROUDED_KEY_BAG_OID: asn1::ObjectIdentifier = | |||
pub const X509_CERTIFICATE_OID: asn1::ObjectIdentifier = asn1::oid!(1, 2, 840, 113549, 1, 9, 22, 1); | |||
pub const FRIENDLY_NAME_OID: asn1::ObjectIdentifier = asn1::oid!(1, 2, 840, 113549, 1, 9, 20); | |||
pub const LOCAL_KEY_ID_OID: asn1::ObjectIdentifier = asn1::oid!(1, 2, 840, 113549, 1, 9, 21); | |||
pub const JDK_TRUSTSTORE_USAGE: asn1::ObjectIdentifier = | |||
asn1::oid!(2, 16, 840, 1, 113894, 746875, 1, 1); | |||
pub const ANY_EXTENDED_KEY_USAGE: asn1::ObjectIdentifier = asn1::oid!(2, 5, 29, 37, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This exists as EKU_ANY_KEY_USAGE_OID
elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood and updated.
src/rust/src/pkcs12.rs
Outdated
#[pyo3(signature = (certs, encryption_algorithm))] | ||
fn serialize_java_truststore<'p>( | ||
py: pyo3::Python<'p>, | ||
certs: pyo3::Bound<'_, pyo3::PyAny>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be possible to just accept a Vec<Py< PKCS12Certificate>>
? (If that's an error, ignore this suggestion :-))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it works! That's much cleaner. Updated.
@@ -388,7 +399,7 @@ enum CertificateOrPKCS12Certificate { | |||
PKCS12Certificate(pyo3::Py<PKCS12Certificate>), | |||
} | |||
|
|||
fn serialize_bags<'p>( | |||
fn serialize_safebags<'p>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this slipped through the cracks for the refactor PR. I'm inclined to leave it in, but it really doesn't matter either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't matter to me
Done. Totally missed that this was a thing. That should cover the requested changes. (Though I'm happy to make others as needed) |
I'll leave the substance to @reaperhulk after we hear from @seanjmullan |
So just to clarify, originally you had?:
|
Yeah, that's what I had put in and tested against. It seemed to behave identically to the "correct" version. OpenJDK seems to only care about the top identifier from what I can tell: https://github.com/openjdk/jdk/blob/b6443f6ff96707f67552df41c01d18c193560223/src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java#L1094-L1097 |
Correct. The top identifier must be '2.16.840.1.113894.746875.1.1'. The inner SET should be a set of ExtendedKeyUsage OIDs, but it currently ignores other OIDs like the above. Adding the any usage OID is correct. |
Following the discussion in #7065, I put together a PR for serializing PKCS12 truststores. This ended up requiring a few more changes than initially expected, but hopefully covers the use-case well enough.
Summary
In short, the current PKCS#12 serialization method (
serialize_key_and_certificates
) was not capable of exporting certs in a way that Java could read as a truststore. In order for be a valid truststore you must:CertBag
typeSafeBags
2.16.840.1.113894.746875.1.1
) on eachCertBag
.Implementation
I initially considered adding this as another argument to
serialize_key_and_certificates
, but it would have cluttered the args and required knowledge of the cert-only constraint. Instead, I've added a newserialize_java_truststore
method which makes the requirements for a truststore much more explicit to the user.As much of the code would have been the same on the rust-side, I ended up refactoring
serialize_key_and_certificates
to pull out a newserialize_safebags
method. This way bothserialize_key_and_certificates
andserialize_java_truststore
would be responsible for creating the safebags needed (and any special attributes), and would pass them off to a shared method for serialization.From the perspective of
serialize_key_and_certificate
the only real change in behavior is that when encryption is not enabled, the key and certificate will be placed in the sameContentInfo
block. This is still valid and should not impact usage in any way.Final Notes
Unit testing to ensure the attribute was added was kinda tricky, as there isn't a reason for this library to parse or store that information. Instead I opted for a golden file, but had to ignore the last 49 bytes to skip the mac digest. (I've added comments explaining this)
If anyone is interested in manually testing a PKCS12 truststore to ensure it's Java compatible, the ideal tool is Java's
keytool
:If the truststore is not valid, it will report "0 entries".
If using something like OpenSSL (
openssl pkcs12 -info -in truststore.p12
), you'd expect to see a section as such: