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."
Also, add the following sentence: "If the cacerts constraint is not set,
then all chains using the specified algorithm are restricted."
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).
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.
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."
74 debug.println("Error parsing cacerts.md");
typo: cacerts.fingerprints
83 decomposer = new AlgorithmDecomposer();
Doesn't seem to be used, delete?
92 public static boolean isAnchor(X509Certificate cert) {
A better name for this method might be contains().
CertificateParameters
---------------------
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?
50 public CertificateParameters(X509CertImpl c, boolean anchor) {
Make private, or just combine with other ctor as it really isn't needed.
59 public boolean isTrustedChain() {
A better name might be "isAnchoredToCaCert".
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".
97 private static final boolean checkerEnabled =
98 certPathDefaultConstraints.checkProperty("cacerts");
Call this cacertsEnabled to be more specific?
159 // Check if the 'cert' is in the trust anchor file (cacerts)
160 private static boolean isAnchor(X509Certificate cert) {
Call this caCertConstraintApplies()?
161 if (!checkerEnabled) {
162 return false;
163 }
I'm not sure I understand why
165 if (debug != null)
debug.println("AlgorithmChecker.isAnchor: " +
Suggest the following to improve readability:
if (debug != null) {
debug.println("AlgorithmChecker.isAnchor: " +
cert.getSigAlgName());
}
DisabledAlgorithmConstraints
----------------------------
62 private final String propertyName;
Doesn't seem to be used; delete?
138 public final void permits(Set<CryptoPrimitive> primitives,
139 X509Certificate cert) throws CertPathValidatorException {
Doesn't seem to be used; delete?
246 if (!constraintsMap.containsKey(algorithm)) {
247 constraintsMap.putIfAbsent(algorithm, new
HashSet<>());
248 }
You can replace this with:
constraintsMap.computeIfAbsent(algorithm, k -> new HashSet<>());
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?
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).
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?
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.
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());
393 public boolean permits(Key key) {
403 public void permits(CertificateParameters cp) throws
CertPathValidatorException {
These should probably be abstract.
416 CertConstraint(String algo, String s) {
Better name for "s"?
--Sean
