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
