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

Discussion: Issuer certificate as (optional) input to lint #428

Open
mtgag opened this issue May 7, 2020 · 11 comments
Open

Discussion: Issuer certificate as (optional) input to lint #428

mtgag opened this issue May 7, 2020 · 11 comments

Comments

@mtgag
Copy link
Contributor

mtgag commented May 7, 2020

Hello,

this came up while working on
#378

The current implementation makes some assumptions because the certificate issuer is not present. An assumption-free implementation would need the issuer certificate as input to the lint.

The proposal is to allow the issuer certificate as (optional) input to lints. This has the advantage that several other lints would then be implementable (e.g. signature verification, AKIE consistency, SubjectDN-issuerDN encoding consistency) also the lint who brought this issue up could be implemented in another way.

What is your opinion on that?

@zakird
Copy link
Member

zakird commented May 7, 2020

I'm certainly not opposed. I think that we should try to write lints such that they can work without the parent when possible, but I see little downside overall, if this means that we can check for additional problems.

@zakird
Copy link
Member

zakird commented May 7, 2020

One issue is that this makes testing a specific certificate non-deterministic. We could incidentally mark a certificate as having an error because we passed in the wrong parent in during the linting process. We should probably have some way of denoting which parent was used for the test in the lint output.

@dadrian
Copy link
Member

dadrian commented May 11, 2020

As @zakird noted, the non-determinism makes this a bit complicated. Assuming the use-cases @mtgag described above (AKID checking, Issuer-Subject DN comparisons), I don’t see us automatically finding the issuer, since to decide which certificate is the issuer, we would be using the aspects of the certificate these lints are specifically checking for errors. Necessarily, we would need to know the “right” answer for the SKID of the parent certificate, the encoding of the Subject DN of the parent, and so on.

Given this, I’d assume anyone using these lints would have a priori decided which certificates to lint against which issuers. One use case might be a CA linting only their own certificates from a specific issuer at time of issuance.

I suggest we take any lints of this form, put them in their own “group”, and assume that ZLint is instantiated with a single issuer to check against, leaving it up to the user to select which certificates to lint, and which issuer to use. We can somehow indicate in the output if we do not think the issuer certificate actually issued an input certificate. We can probably also support one issuer per key type, e.g. one RSA cert, one ECDSA certificate, etc.

If someone wishes to send all certificates ever issued through the lints requiring an issuer, it would be “on them” to figure out which issuer to use with which certificate.

@zakird
Copy link
Member

zakird commented May 11, 2020

I agree with @dadrian points out here, but also note that CAs add the parent to CT, so in many ways we know the issuing certificate that CAs intended to use when performing external validation.

@mtgag
Copy link
Contributor Author

mtgag commented May 12, 2020

I thought about a simple approach. Something like extending the LintInterface to

CheckApplies(c *x509.Certificate, issuer *x509.Certificate) bool and

Execute(c *x509.Certificate, issuer *x509.Certificate) *LintResult

where the issuer is optional.

For old lints (that do not need the issuer) nothing changes in the implementation independent of whether the caller provides the issuer or not. For lints that require the issuer a simple check in CheckApplies like

if (issuer is not provided) return false
perform other checks to evaluate whether this lint should be triggered or not.

and in Execute the issuer is evaluated (because it definitely present) to implement the lint.

To sum it up. If the LintInterface is extended with the optional issuer, the caller has an additional chance to provide the issuer to enable further lints. If the caller does not provide the issuer then the current functionality of the lints remains unchanged. In terms of source code the LintInterface and the underlying lints needs to be extended with the new argument and all calls to CheckApplies and Execute provide a nil as a second argument. The implementations of lints must not be changed at all.

The support for the issuer is then provided by zlint. It is up to the callers to take advantage of this functionality and perform this correctly. I am not sure whether this is a task for zlint. What do you think about this approach?

@cardonator
Copy link
Contributor

I agree with @dadrian that the lints being non-deterministic and/or relying on what could be third party information makes them a better candidate for a separate group of lints.

Also, I feel like it's pretty rough to change the Lint interface for what is likely to be decidedly less lints. That means every lint needs to be able to handle an issuer parameter even if it won't ever use it, which will be the case the vast majority of the time. It also means every lint implementation would have to change in one PR. Yuck!

Unfortunately, my first statement above would also mean that very few people would bother with these lints. Also, the added complexity to the testCerts model currently used in zlint can't be ignored.

@dadrian
Copy link
Member

dadrian commented May 12, 2020

CheckApplies(c *x509.Certificate, issuer *x509.Certificate) bool
Execute(c *x509.Certificate, issuer *x509.Certificate) *LintResult

What happens when issuer is nil for a lint that requires an issuer? Does it skip CheckApplies, or is it an error result?

@zakird
Copy link
Member

zakird commented May 12, 2020

That's a good question. I think that might very well introduce a new status similar to NA but for not enough data provided result. Right now the options are:

const (
.

@mtgag
Copy link
Contributor Author

mtgag commented May 13, 2020

I believe if the issuer is nil this should be a "weak" failure. Either skip in the sense that this lint does not apply since the issuer is missing or as @zakird proposed introduce a new status. Otherwise providing the issuer would become somehow mandatory which currently I think it is restrictive.

@zakird
Copy link
Member

zakird commented May 13, 2020

My worry for NA is that I don't want folks to think they've had all applicable tests run when that hasn't actually happened. A certificate could be in scope and we just weren't able to run the lint.

@christopher-henderson
Copy link
Member

#include <discussion_from_491>

@sleevi @cardonator @zakird porting the week's coversation from #491 here trying to break up the conversation a bit.

There appears to be a second, equally complex, discussion of what to do with otherwise ambiguous results (because even if we allow for a chain to be included in the run that does not mean that it always will be).

So I went ahead and fired up #522 to discuss precisely what-and-how to convey the notion that a lint result was inconclusive.

Hopefully that can help narrow this particular thread to only issues of, say, path building.

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

No branches or pull requests

6 participants