RFR 8140422: Add mechanism to allow non default root CAs to be not subject to algorithm restrictions

[Anthony Scarpino]

On 03/08/2016 01:55 PM, Sean Mullan wrote:
> You should also copy build-dev on the next iteration since there are a
> few Makefile changes.
>
> On 02/29/2016 11:55 AM, Anthony Scarpino wrote:
>> I need a code review of this change:
>>
>> http://cr.openjdk.java.net/~ascarpino/8140422/webrev/
>>
>> Currently CertPath algorithm restrictions allow or deny all
>> certificates.  This change adds the ability to reject certificate chains
>> that contain a restricted algorithm and the chain terminates at a root
>> CA; therefore, allowing a self-signed or chain that does not terminate
>> at a root CA.
>>
>> https://bugs.openjdk.java.net/browse/JDK-8140422
>
> Looks good, but had a few comments as I went through ...
>
> java.security
> -------------
>
>   551 #   "cacerts" prohibits the algorithm only if it is used in a
>   552 #     certificate chain that terminates at a distribution provided
>   553 #     trust anchor in the lib/security/cacerts keystore. All other
>   554 #     chains are not affected. Example: to apply this constraint
>   555 #     to SHA-1 certificates, include the following:  "SHA1 cacerts"
>
> Suggest rewording in middle part: "... at a trust anchor in the
> lib/security/cacerts.fingerprints file which is initially seeded with
> the fingerprints of the certificates in the lib/security/cacerts keystore."

ok

>
> Also, add the following sentence: "If the cacerts constraint is not set,
> then all chains using the specified algorithm are restricted."

ok

>   563 # All DisabledAlgorithms are processed in the order defined in the
>   564 # property.  This requires lower keysize constraints to be specified
>   565 # before larger keysize constraints of the same algorithm.
>
> Is this still an issue? Constraints of the same algorithm are evaluated
> as an AND, so the order should not matter (I think).

My usage of "DisabledAlgorithms" is the top level, comma delimited of 
the constraints as defined by the syntax at the top of the section.
If a user says :
"RSA keySize < 2048, RSA keySize < 1024 & cacerts",  The first 
DisabledAlgorithm is going to trigger failures before the second 
DisabledAlgorithm can be evaluated

Previous discussion of AND was referring to constraints inside of 
DisabledAlgorithm.  "RSA keySize < 1024 & cacerts" is an AND operation 
as both constraints have to pass pass.

>
> CACertsHasher
> -------------
>
> I think it would be useful to emit the subject DN of the certificate as
> a comment (ex: "# CN=RootsRUs, ...") above each fingerprint, as it would
> help associate the fingerprint with the root. Then, you can ignore
> comments when you parse the file in AnchorCertificates.

ok

>
> AnchorCertificates
> ------------------
>
>    38 /**
>    39  * The purpose of this class is to determine if a trust anchor
> certificate
>    40  * is one of the default trust anchors that comes pre-installed
> with the JRE
>    41  * in the cacerts file. A locally installed trust anchor would not
> be a default
>    42  * trust anchor
>    43  */
>
> The determination is based off the cacerts.fingerprints file, so maybe a
> better wording is: "The purpose of this class is to determine if a trust
> anchor certificate is in the cacerts.fingerprints file which is
> initially seeded with the fingerprints of the certificates in the
> cacerts file. A trust anchor that is subsequently added to the cacerts
> file but not the cacerts.fingerprints file would not be a match."
>

ok


>    74                         debug.println("Error parsing cacerts.md");
>
> typo: cacerts.fingerprints

yep

>
>    83         decomposer = new AlgorithmDecomposer();
>
> Doesn't seem to be used, delete?

delete

>
>    92     public static boolean isAnchor(X509Certificate cert) {
>
> A better name for this method might be contains().

sure


>
> CertificateParameters
> ---------------------
disagreement inline, otherwise comment accepted

>
>    37 public class CertificateParameters {
>
> A better name might be CertConstraintParameters.
>
>    43         this(X509CertImpl.toImpl(c), anchor);
>
> It doesn't seem like you need to convert this to an X509CertImpl. In
> DisabledAlgorithmConstraints, you just call methods on X509Certificate.
>
>    46     public CertificateParameters(X509Certificate c) throws
> CertificateException {
>
> Doesn't seem to be used; remove?

It's used in DisabledAlgorithmConstraints in 138

>
>    50     public CertificateParameters(X509CertImpl c, boolean anchor) {
>
> Make private, or just combine with other Constructor as it really isn't needed.
>
>    59     public boolean isTrustedChain() {
>
> A better name might be "isAnchoredToCaCert".

I'm changing this to isTrustedMatch to be consistent across 
AlgorithmChecker, CertConstraintsParameters, and 
DisabledAlgorithConstraints.
>
>    63     public X509CertImpl getCertificate() {
>
> Change return type to X509Certificate.
>
> AlgorithmChecker
> ----------------
>
>    93     // True if trust anchor checking finds a match in the cacerts
> file
>    94     private boolean trustedMatch = false;
>
> This really means "true if the cacerts constraint enabled AND the chain
> is  anchored by a cacert". Can you add that to the comment so it is more
> clear? Maybe the variable name should be changed to
> "caCertConstraintApplies".

I added the comment, but left the variable name as I made it consistent 
through out as previous mentioned
>
>    97     private static final boolean checkerEnabled =
>    98             certPathDefaultConstraints.checkProperty("cacerts");
>
> Call this cacertsEnabled to be more specific?

Sure

>
>   159     // Check if the 'cert' is in the trust anchor file (cacerts)
>   160     private static boolean isAnchor(X509Certificate cert) {
>
> Call this caCertConstraintApplies()?

I think checkFingerprint() is better.  Trying to get cacerts into the 
name is too messy
>
>   165         if (debug != null)
> debug.println("AlgorithmChecker.isAnchor: " +
>
> Suggest the following to improve readability:
>
>      if (debug != null) {
>          debug.println("AlgorithmChecker.isAnchor: " +
>                        cert.getSigAlgName());
>      }

ok

> DisabledAlgorithmConstraints
> ----------------------------
>
>   62     private final String propertyName;
>
> Doesn't seem to be used; delete?

yes

>
>   138     public final void permits(Set<CryptoPrimitive> primitives,
>   139             X509Certificate cert) throws CertPathValidatorException {
>
> Doesn't seem to be used; delete?

it's used.

>
>   246                     if (!constraintsMap.containsKey(algorithm)) {
>   247                         constraintsMap.putIfAbsent(algorithm, new
> HashSet<>());
>   248                     }
>
> You can replace this with:
>
>      constraintsMap.computeIfAbsent(algorithm, k -> new HashSet<>());

ok

> Same comment on lines 280-282.
>
> 251                 algorithm.toUpperCase(Locale.ENGLISH);
>
> Shouldn't you also do this before adding it to the map on line 246-7?

yes.. some other changes can be done around this
>
>   253                 if (debug != null) debug.println("Constraints len:
> " +
>   254                         rComma.substring(space).split("&").length);
>
> I suspect this debugging is more useful for debugging your own code. But
> we shouldn't include it unless it is helpful for analyzing
> real issues, as the debug log files are long enough :) Same comment
> applies to other debugging statements (ex: lines 324-9).

Sure.. I had been debating on removing these

>
> 287                     lastConstraint = c;
>
> This seems like a bug since it is overwriting what was just done on line
> 285. Should this be moved to right after line 283?

I believe this is correct.  lastConstraint is suppose to be the last one 
in the linked list.  285 is adding the new constraint to the next link 
in the linked list.  287 is setting the new constraint as the 
lastConstraint.  If I changed it after 283, it would keep overwriting 
the next link the list.  The linked is how I tell it's an AND operation 
vs a new constraint.

>
> Also, if you had a constraint like: "RSA keySize < 1024, RSA keySize >
> 4096" it looks like the 2nd RSA constraint will replace the first
> constraint, which isn't what I think is intended.

The map is <Algorithm, HashSet<Constraint>>
So it looks like <RSA, [keySize < 1024, keySize < 4096]>

That is why getConstraints() returns a Set.

>
>   293         public Set<Constraint> getConstraints(Key key) {
>
> I think you can remove this method and just change line 304 to:
>
>      Set<Constraint> set = getConstraints(key.getAlgorithm());

sure

>
>   393         public boolean permits(Key key) {
>   403         public void permits(CertificateParameters cp) throws
> CertPathValidatorException {
>
> These should probably be abstract.

They meant to be inherited if the Constraint doesn't require the check. 
  In the case of 393, it is used by CertConstraint.
>
>   416         CertConstraint(String algo, String s) {
>
> Better name for "s"?

not applicable anymore.

>
> --Sean