NPE in serverHandshaker.

[sbernard31]

If CertificateVerifier.getAcceptedIssuers() returns null this generate an NPE.

unexpected error occurred:
java.lang.NullPointerException: null
	at java.util.Objects.requireNonNull(Objects.java:203)
	at java.util.Arrays$ArrayList.<init>(Arrays.java:3813)
	at java.util.Arrays.asList(Arrays.java:3800)
	at org.eclipse.californium.scandium.dtls.ServerHandshaker.createCertificateRequest(ServerHandshaker.java:532)
	at org.eclipse.californium.scandium.dtls.ServerHandshaker.receivedClientHello(ServerHandshaker.java:400)
	at org.eclipse.californium.scandium.dtls.ServerHandshaker.doProcessMessage(ServerHandshaker.java:200)
	at org.eclipse.californium.scandium.dtls.Handshaker.processMessage(Handshaker.java:638)
	at org.eclipse.californium.scandium.DTLSConnector.startNewHandshake(DTLSConnector.java:1842)
	at org.eclipse.californium.scandium.DTLSConnector.processClientHello(DTLSConnector.java:1737)
	at org.eclipse.californium.scandium.DTLSConnector.access$1500(DTLSConnector.java:217)
	at org.eclipse.californium.scandium.DTLSConnector$12.run(DTLSConnector.java:1675)
	at org.eclipse.californium.elements.util.SerialExecutor$1.run(SerialExecutor.java:276)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)

The javadoc says :

/**
 * Return an array of certificate authority certificates which are trusted
 * for authenticating peers.
 * 
 * @return the trusted CA certificates (possibly <code>null</code>)
 */
X509Certificate[] getAcceptedIssuers();

So there is bug either in javadoc or in the code ?

My bet is that allowing null make sense as it seems optional to me to provide AcceptedIssuers. (I should double check this)

If there is a bug in the code we should fix it for the 2.2. (#1244)

[sbernard31]

Reading rfc5246§7.4.4, I would say this is an issue in the code.

certificate_authorities

A list of the distinguished names [X501] of acceptable
certificate_authorities, represented in DER-encoded format. These
distinguished names may specify a desired distinguished name for a
root CA or for a subordinate CA; thus, this message can be used to
describe known roots as well as a desired authorization space. If
the certificate_authorities list is empty, then the client MAY
send any certificate of the appropriate ClientCertificateType,
unless there is some external arrangement to the contrary.

[sbernard31]

@boaks if you ok I will provide a fix with a null check tomorrow ?

[boaks]

I would prefer to adapt the javadoc of the CertificateVerifier.getAcceptedIssuers().

I had X509TrustManager.getAcceptedIssuers() in mind, when I implemented it. And so I also defined DtlsConnectorConfig.Builder.setTrustStore accordingly.

Empty array = trust all, don't send issuers.
null := not intended, error.

[boaks]

The RFC just defines, that a empty list is valid (in my experience, especially for systems, which supports many self-signed certificates, that's the only way to go).
But I would not interpret this as "null" is OK for a empty issuer list.

[sbernard31]

We agree that the rfc just says that the list can be empty.

So allow null and/or empty is an API question.

For trustStore I can understand :

• empty means trust all
• null meams trust nothing
• something means trust this.

But for ceritifcateVerifier I don't get the benefits to force implementer to return an empty list for acceptedIssuer ? why not considering null like empty ?

[boaks]

But for ceritifcateVerifier I don't get the benefits to force implementer to return an empty list for acceptedIssuer ?

FMPOV, someone who reads the (corrected :-) )javadoc should be able to provide a empty array instead of null. That may ensures, that "trust all" is not chosen accidentally.
And it's also not too uncommon, though, as you have read, java defines that for its issuer list accordingly. Or why should this implementation differ from java without real reason?

[boaks]

if (config.getCertificateVerifier().getAcceptedIssuers() != null) {
	int index = config.isSendRawKey() ? 1 : 0;
	this.supportedClientCertificateTypes.add(index, CertificateType.X_509);
}

That also part of the commit introducing the CertificateVerifier. With null it seems to even disable x.509.

[sbernard31]

• We could consider that force users to create an empty array to then convert it in a list to then doing nothing is "surprising".
• I do no understand how someone could "trust-all" accidentally when he implements CertificateVerifier ?
• I do not understand why doing like the JVM API is important ?
• This now in the API and should not be changed for a minor release

About your last comment, I didn't understand could you give me a link to the code ?

[boaks]

About your last comment, I didn't understand could you give me a link to the code ?

98675f3#diff-0f4638ef3b80663963d3545f6910623bR173

[boaks]

@sbernard31

I would prefer a "empty array" but if you insist in null, go for it.

[boaks]

Just to mention: support null may require more changes, e.g. calls to CertPathUtil.
As I wrote above, using the empty array was considered from me to be the right approach.

[sbernard31]

As a user I just respected the javadoc to implement leshan-server-demo and my code doesn't work. (Since I discover "this bug", I write the code in a different way now)

I will not insist, I just don't see (for now) any good argument to not allow null and I see some (not strong) arguments to support it, so I would rather go for support null.

About 98675f3#diff-0f4638ef3b80663963d3545f6910623bR173 : I didn't see it as an argument. It could be considered as a bug.

About CertPathUtil, I didn't get the point. This is called only by StaticCertificateVerifier, why should we change this ?

Maybe there is a miss understanding but I see getAcceptedIssuers as a getter to fill the certificate_authorities of certificate request just this. I don't want to change any others logic.

[boaks]

There are either bugs in the implementation or the javadoc.

As I tried to explain, I'm used to the definitions of the java Trust API, and so my implementation assume a "empty array". Changing that to support null may cost more effort (and for the next time uncertainness) then changing the javadoc.

About 98675f3#diff-0f4638ef3b80663963d3545f6910623bR173 : I didn't see it as an argument. It could be considered as a bug.

With that we have two bug in the code or one in the javadoc.

About CertPathUtil, I didn't get the point. This is called only by StaticCertificateVerifier, why should we change this ?

Maybe there is a miss understanding but I see getAcceptedIssuers as a getter to fill the certificate_authorities of certificate request just this. I don't want to change any others logic.

If user gets used to use the null, I assume that they (want to) use it in other similar cases. At least for me it seems to be obvious to apply similar rules for similar cases.

[sbernard31]

Changing that to support null may cost more effort (and for the next time uncertainness) then changing the javadoc.

I don't see this cost as getAccepterIssuers is called only 1 time and just to fill certificate_authorities`

With that we have two bug in the code or one in the javadoc.

I don't see that as those lines of code seems to not exist anymore.

If user gets used to use the null, I assume that they (want to) use it in other similar cases. At least for me it seems to be obvious to apply similar rules for similar cases.

I disagree as I don't consider this is similar cases. Again I understand your choice for truststore but here this is different to me.

But anyway I don't think this details deserve more time, do what you prefer.

[boaks]

But anyway I don't think this details diverse more time, do what you prefer.

OK, so I go for change/adapt the javadoc.