Add lint to check encoding of SubjectPublicKeyInfo.AlgorithmIdentifier in S/MIME certificates#986
Add lint to check encoding of SubjectPublicKeyInfo.AlgorithmIdentifier in S/MIME certificates#986defacto64 wants to merge 44 commits intozmap:masterfrom
Conversation
Added //nolint:all to comment block to avoid golangci-lint to complain about duplicate words in comment
Fixed import block
Fine to me. Co-authored-by: Christopher Henderson <[email protected]>
As per Chris Henderson's suggestion, to "improve readability".
As per Chris Henderson's suggestion.
Added CABFEV_Sec9_2_8_Date
christopher-henderson
left a comment
christopher-henderson
left a comment
There was a problem hiding this comment.
I went down this whoooole rabbit hole about how we have another lint that checks for this, and how I can't find any reference to the Edward curves. and also where are you seeing ANY reference at all to the PQC algorithims...and...and....
Oh, this is the SMIME BRs 🫠
Thank you! I do have one request, though, to split the PQC algorithms into a separate lint as to avoid accidentally sunsetting them with the legacy algorithms.
| } | ||
|
|
||
| // Let's now try with the PQC algorithms | ||
| if !c.NotBefore.Before(pqcEffectiveDate) { |
There was a problem hiding this comment.
Having two efficacy dates within one lint hints to me that perhaps this should be two lints. It's a bit of a pain-in-the-butt, but would you mind splitting the PQC algorithms into a separate lint?
My reasoning is that if/when the legacy algorithms are marked Ineffective that we will cease to check for PCQ algorithms that were created after the sunsetting of the legacy algorithms.
It's tough to visualize, but this is what I mean,
- PQC accidentally gets sunsetted along with the legacy algorithms.
|------|
legacy --> pqc --> ineffectiveLegacy ---> .....
- As a separate lint, PQC keeps going after quantum computing and AGI makes this all a pointless as we are now either in a utopia or in the Matrix.
|---------------------------------------------|
legacy --> pqc --> ineffectiveLegacy ---> .....
There was a problem hiding this comment.
I am a bit puzzled. The logic behind this lint is to take the binary encoding (DER) of the SPKI algorithm and see if it appears in a list of allowed encodings (a white list). If I split the PQC algorithms into a separate lint, as you suggest, it's not clear to me how can I avoid to return an error — from this separate lint — if the algo's binary encoding doesn't appear in the relevant white list but nonetheless is legit (as in the case of a legacy algo). And similarly, in reverse, for the first lint, which is supposed to only handle legacy algos. In short, it's not clear to me how to do what you're asking.
There was a problem hiding this comment.
I've taken a crack at what I am thinking in #989. The two lints in question are...
Legacy Algorithms Only
Effective: CABF/SMIME 1.0.0
Ineffective: CABF/SMIME 1.0.11
Checks for the presence of legacy algorithms only. PCQ algorithms were not yet allowed in this window, so PQC certs issued during this time should fail.
Legacy and PCQ Algorithms
Effective: CABF/SMIME 1.0.11
Allows for either legacy or PCQ algoids, but only for certs issued on-or-after 1.0.11.
|
OK. So, first, I've modified this lint to have the effective date of SMIME BR 1.0.11, which introduced the PQC algorithms, and thus removed the additional check for the certificate issuance date I'd previously added. I've also trimmed down the test data a bit, skipping some unnecessary tests, and at this point I think this lint meets your expectations for the current stage (post-SMIME BR 1.0.11 release). If you're okay with it, I'll later submit a PR for a second lint called "... _legacy" that will only address the algos that were allowed in the previous stage. |
|
@christopher-henderson While I was updating this PR, I didn't realize you'd decided to take the initiative and edit my lint yourself (and then submit a separate PR). It's all the same to me; go ahead as you prefer. If you like, I'll close this PR. |
|
Thank you @defacto64! I've merged #989 so I'll close this one out. |
2 participants
This lint checks that the DER encoding of the AlgorithmIdentifier (within SubjectPublicKeyInfo) is allowed, namely that it complies with section 7.1.3 of CABF SMIME BRs, also taking into account the allowance for PQC algorithms introduced on August 22 (see CABF SMIME BRs version 1.0.11).