Conversation
Fix defaults for keyType, keyLength, keyCurve, signatureAlgorithm
… handlers don't properly deal with platform differences (e.g. /C:/ path). Fixed bugs in certificate and other generators because bouncy castle doesn't properly handle whitespace per RFC7468. Fixed typos in comments. Set default signature algorithm to SHA256.
djivko
left a comment
djivko
left a comment
There was a problem hiding this comment.
Review seems good. There are a few suggestions regarding the code style - common methods etc.
| ECPublicKey reqEcPublicKey = (ECPublicKey) keyPair.getPublic(); | ||
|
|
||
| // https://stackoverflow.com/questions/24121801/how-to-verify-if-the-private-key-matches-with-the-certificate | ||
| java.security.spec.ECParameterSpec certSpec = certEcPublicKey.getParams(), csrSpec = reqEcPublicKey.getParams(); |
There was a problem hiding this comment.
This check and the one bellow are identical worth extracting common method for those as they are not trivial.
| String pem = null; | ||
| if (!Objects.isNull(this.certificate)) { | ||
| ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); | ||
| try (PemWriter pemWriter = new PemWriter(new OutputStreamWriter(outputStream))) { |
There was a problem hiding this comment.
I would extract a common method - private String toPem(String type, byte[] encoded) { .... }. The same code is repeated through out the class.
| .stream() | ||
| .filter(Objects::nonNull) | ||
| .map(entry -> new SANItem().type(type).name(entry.toString())) | ||
| .map(entry -> new SANItem().type(type).name( type == 7 ? ((InetAddress)entry).getHostAddress() : entry.toString()) ) |
There was a problem hiding this comment.
What about - entry instanceof InetAddress ? ((InetAddress)entry).getHostAddress() : entry.toString.
| request.keyCurve(EllipticCurve.ellipticCurveDefault()); | ||
| } | ||
| if (request.signatureAlgorithm() == SignatureAlgorithm.UnknownSignatureAlgorithm) { | ||
| request.signatureAlgorithm(SignatureAlgorithm.ECDSAWithSHA256); |
There was a problem hiding this comment.
What about having defaultECAlg and defaultRSAAlg methods in SignatureAlgorithm class. This will allow us to easily change defaults if ever needed.
|
|
||
| default: | ||
| if (request.keyLength() < 2048) { | ||
| request.keyLength(2048); |
There was a problem hiding this comment.
Some common place to define the default Length
| @@ -74,47 +93,14 @@ public void updateCertificateRequest(CertificateRequest request) { | |||
| } | |||
| import java.util.Base64; | ||
| import org.bouncycastle.asn1.x509.GeneralName; | ||
| import org.bouncycastle.asn1.x509.GeneralNames; | ||
| import org.bouncycastle.asn1.x509.X509Extension; |
There was a problem hiding this comment.
This and import below seems to be deprecated, so probably better to replace with org.bouncycastle.asn1.x509.Extension
| import org.bouncycastle.asn1.eac.ECDSAPublicKey; | ||
| import org.bouncycastle.asn1.x500.X500NameBuilder; | ||
| import org.bouncycastle.asn1.x500.style.BCStyle; | ||
| import org.bouncycastle.jce.PKCS10CertificationRequest; |
There was a problem hiding this comment.
seems deprecated and should be imported from org.bouncycastle.pkcs
| } | ||
|
|
||
| GeneralNames names = new GeneralNames(sans.toArray(new GeneralName[] {})); | ||
| Vector oids = new Vector(); |
There was a problem hiding this comment.
Vector is a raw type and better to be parameterized. Aplpy for the line below also.
| ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); | ||
| try (PemWriter pemWriter = new PemWriter(new OutputStreamWriter(outputStream))) { | ||
| pemWriter.writeObject(new PemObject("CERTIFICATE", this.certificate.getEncoded())); | ||
| } catch (CertificateEncodingException e) { |
There was a problem hiding this comment.
You can handle both exception here, i.e catch (CertificateEncodingException | IOException e)
5 participants
No description provided.