-
Notifications
You must be signed in to change notification settings - Fork 100
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
Verify with X.509 Certificate #1139
Conversation
3966617
to
b3ef19f
Compare
cce0b06
to
57b67e3
Compare
The verification seems to ignore the difference between root and intermediate certificates. For instance, the following snippet succeeds while it shouldn't (the certificates are swapped).
|
57b67e3
to
07586a4
Compare
I'd expect singularity verify \
--certificate ./test/certs/leaf.pem \
--certificate-roots ./test/certs/intermediate.pem \
../images/busybox_latest.sif To demonstrate that they're treated differently, note that this is not equivalent to: singularity verify \
--certificate ./test/certs/leaf.pem \
--certificate-intermediates ./test/certs/intermediate.pem \
../images/busybox_latest.sif Here, the same chain If you take it the other way and specify singularity verify \
--certificate ./test/certs/leaf.pem \
--certificate-roots ./test/certs/root.pem \
../images/busybox_latest.sif All that being said... this code is quite green and there's always a possibility I've messed something up! Let me know if the above makes sense... or if you feel there's an issue with it. |
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 LGTM subject to any further input from @fnikolai
One request - as we now have useful functionality exposed here, please could you add an entry / entries to the CHANGELOG.md covering the sign/verify additions?
Add OptVerifyWithCertificate, OptVerifyWithIntermediates, and OptVerifyWithRoots options to support X.509 implementation.
@tri-adam I see your point. However, I 'm still not sure if this is the desired behavior. In the sense that if I can assign any of the intermediates to be the root, then I can effectively skip the rest of the certificate chain. As a result, if any certificate higher in the chain is revoked, the revocation check can be easily bypassed. Going a bit further, I think the issue originates from gen_certs, where all the certificates are signed from the same CA key. (Shouldn't we use the intermediate's key for signing the leaf certificate ?)
On another note related to #1152, if we use the chain produced by Certificate.Verify (here) as hinted on For that, I 'm now working on a tiny OCSP_Responder to complete the gen_certs.go |
On the good side, we now have:
On the bad side, the hashing algorithms of the original testing keys (leaf, intermediate, private) appear to be incompatible with OCSP. For OCSP only SHA1, SHA224, SHA256, SHA384, and SHA512 are allowed. That given, I had to create new keys, which means that now I have to re-sign the testing images. Is there any scripted way to do that? Once the images are re-signed, I 'll make PR of the commit mentioned above. |
If the user specifies certificate(s) via
Thus, as long as it is possible to build a sequence of certificates that satisfy the algorithm in RFC 5820 § 6.1:
using a certificate specified in
Yeah, I was being a bit lazy there and just using the same key since we only have one in our corpus. I'll add a couple more keys and update the cert generation code to use different keys for each cert it generates.
Makes sense. I don't see an easy way to test end-to-end without a container that was signed with a private key for which we have a valid cert chain. |
Hmm, that's interesting. The current cert is generated using Ed25519 keys, which always uses SHA512 when signing (ref). What encryption algorithm are you using for the keys you have generated? Can you share the error(s) are you getting when using OCSP with the current cert? |
2c81f8d
to
1e77d4c
Compare
To generate keys:
and then:
In both cases, I 've removed and re-created the certs using |
Weird. Difficult to imagine OCSP doesn't support Ed25519, but we can switch to RSA if it's easier in the short term. Once you've got OCSP working with RSA, I would like to figure out why Ed25519 isn't working... One more note... the DSSE image in the corpus ( Thanks again for all the help reviewing this. Very much appreciated! |
1e77d4c
to
a40013b
Compare
I've proposed adding RSA and ECDSA keys in #1183 and rebased this PR on top of that. Different keys are now used for each of the three certs in |
I have the feeling it will fail :(. The reason is that OCSP server responds with the status of the certificate in question, and the certificate of the server itself (which must be also validated). In this case, the response for (leaf) will be (status, root). And if we try to validate the root, it will fail because the certificate is signed using Ed25519 keys. I ll check it tomorrow. |
21873bf
to
0a914e4
Compare
OK, yeah let me know how it goes. The test cert chain is now, for better or worse, going to exercise the three main types of key material that people will realistically use in the wild. Hopefully we can get OCSP working with all three, but if there's some technical reason why this isn't possible, we'll figure out a path forward. The |
@tri-adam - are we in a position to get this merged at this point? If anything is needed around test keys later on, with regard to OCSP, we can adapt it in a later PR. I think this PR is internally consistent and LGTM |
From my perspective, yes. The main question left is around the cert chain being suitable for OCSP, I believe. From my perspective, it might be better to have those changes in a PR next to the code that requires them, for ease of review. @fnikolai does that sound good to you? |
Yeap, I agree that it's time to merge this PR, and continue the OCSP discussion on #1152 |
Description of the Pull Request (PR):
Add test certificate chain. Add X.509 flags and environment variables to
singularity verify
command.This fixes or addresses the following GitHub issues: