Add CAs/CRLs to the PKI engine at runtime #3606
Conversation
gerardsn
requested review from
woutslakhorst,
reinkrul and
stevenvegt
as code owners
gerardsn
force-pushed
the
fix-3583/seperate-CRL-checking-from-tlsConfig-generation
branch
from
a3661b7 to
c7c8424
Compare
Through PEX you have control over what DIDs you resolve on that part. But now I think of it, we might resolve DID in other places, which is not filtered through PEX/PDs first? Maybe This actually also applies to |
|
I think this is a major concern. We currently have no limitations on when DIDs are allowed to be resolved. I'm assuming there are at least several (if not all) cases where this is not preceded by a check on what DID is being resolved. For example, dereferencing compound services in did:nuts. These can point to a bunch of did:x509 DIDs to rapidly fill the CRL checker with garbage. I know this is a deprecated feature, but the fact that there is no restriction on resolving makes this very vulnerable. I wonder if the CRL check should take place in the X509Credential validation? This is a revocation check, which does not exist for other DIDs? Although you could argue that this is similar to deactivation for DIDs. I don't think this applies to |
|
Only a few (maybe even 1) malicious CRL endpoints in the list is enough to break the CRL checker. Currently all CRL endpoints are checked every 15 minutes in order (no parallelism). |
Isn't this a major issue as well, then? ANd when we fix this, the CRL issue still applies? |
| return err | ||
| } | ||
| if trustStore != nil { | ||
| err = p.addCAs(trustStore.Certificates()) |
There was a problem hiding this comment.
this could theoretically add leaf certificates as wel? Maybe better to only add IntermediateCA and RootCA certificates? (I think it has fields for that).
There was a problem hiding this comment.
true, but this will only be the case for the first leaf cert presented for an unknown chain. So it just adds a bit of data per issuer that cannot do any harm. It does allow prefetching of its issuers CRL since that can only be found on the leaf cert.
We use the hosts truststore for did:web. If you want to do CRL checking on ALL public CAs it's no longer part of the nuts node IMO. |
I'll create an issue for it, since CRL checking is i.m.o. an important part of the security model of |
gerardsn
force-pushed
the
fix-3583/seperate-CRL-checking-from-tlsConfig-generation
branch
from
23346a6 to
db83268
Compare
|
Following the discussion above I have removed the CRL check form the did:x509 resolver since this would allow unsanitized user input into the CRL database. CRL checking must be done in the VC validation -> it is a credential revocation check. |
gerardsn
deleted the
fix-3583/seperate-CRL-checking-from-tlsConfig-generation
branch
I've updated some go docs and comments to clarify that the pki engine is only a CRL checker, not a trust manager.
Since trust in the certificate chain must be validated externally (prior to checking the CRLs), we can assume that all presented certificates are part of trusted certificates chains and can be added to the CRL/CA lists at runtime. This simplifies the usage of the PKI engine a bit and closes #3583.
PKI.CreateTLSConfig()returns atls.Configthat contains the truststore that is in the nuts.yaml config. Added clarification that this must not be replaced the internal truststore of the engine. This method only exists here to set the CRL checker on thetls.Config.VerifyPeerCertificate.Currently the CRLs are checked by the did:x509 resolver that, I think, does not know if the chain should be trusted or not?
This means that the CRLvalidator / PKI engine will be filled based on data/certs presented by external parties -> enables certain attacks on the node.
TODO: