Support multiple FIDO MDS trust roots - #503
Conversation
8ed9d58 to
4a24408
Compare
| return Collections.singletonList(trustRootCertificate); | ||
| return trustAnchors.stream() | ||
| .map(TrustAnchor::getTrustedCert) | ||
| .findFirst() |
There was a problem hiding this comment.
Not sure there is a real world scenario for this
but it looks like there's a gap that should at least be documented.
Assuming an mds blob is loaded with no x5u or x5c in the header, the first cert will be picked, despite there potentially being multiple certs in the trust anchors.
Now, in a scenario where a library user parsing an mds blob (with no x5u or x5c in the header) that has a signing cert about to expire, might try to add both the new and old signing certificate to the trust anchors, but the cert returned here will always give the first cert... (meaning the second cert will never be used)
Shouldn't this method rather return an empty list in this case?
The logic in verifyBlob would have to detect this, and instead try each trust anchor, attempting to use those directly to verify the signature without an intermediate chain.
The implementation of verifyBlob would look something like:
final MetadataBLOBHeader header = parseResult.blob.getHeader();
final List<X509Certificate> certChain = fetchHeaderCertChain(trustAnchors, header);
final List<X509Certificate> leafCerts;
if(certChain.isEmpty()) {
leafCerts = trustAnchors.stream()
.map(TrustAnchor::getTrustedCert)
.collect(Collectors.toList());
} else {
leafCerts=Collections.singletonList(certChain.get(0));
}
for (final X509Certificate leafCert : leafCerts) {
... existing leafCert processing ...
}
throw new IllegalStateException(
"Exited without finding a certification path or failing to validate any certification path. This should be impossible, please file a bug report.");
There was a problem hiding this comment.
Yes, thanks for the call-out! I think it's unlikely to come up in practice, so I just did an ugly placeholder solution... but you're right that we can probably do this better without too much trouble by adding some kind of retry loop for this case.
2 participants
This will help to better work around issues like #498 in the future.