Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose the certificate chain in the sender Identity #493

Closed
wants to merge 1 commit into from

Conversation

msangoi
Copy link
Contributor

@msangoi msangoi commented Mar 28, 2018

No description provided.

@AlexITC
Copy link
Contributor

AlexITC commented Mar 28, 2018

I feel that I have already merged something like this.

@AlexITC
Copy link
Contributor

AlexITC commented Mar 28, 2018

Found it, it seems that I forget to upload the pull request (commit).

return Identity.x509(peerAddress, x509CommonName);
} else if (senderIdentity instanceof X509CertPath) {
return Identity.x509(peerAddress, ((X509CertPath) senderIdentity).getPath());
Copy link
Contributor

@AlexITC AlexITC Mar 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't useful to have the common name and the certificate chain when possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The common name is extracted from the first certificate of the chain in the Identity class.

@@ -52,6 +52,7 @@ public static JsonObject serialize(Identity identity) {
o.set(KEY_RPK, Hex.encodeHexString(publicKey.getEncoded()));
} else if (identity.isX509()) {
o.set(KEY_CN, identity.getX509CommonName());
// we don't store the certificate chain if it is available in the identity
Copy link
Contributor

@AlexITC AlexITC Mar 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the certificate chain is needed only when the session is established, in order to have a custom verification of the client certificate. Once it is verified, it is probably not needed anymore and storing it is quite costly in terms of payload size.
Do you have a use case for storing it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I added the changes, I was serializing the certificate chain, honestly, I can't remember why.

I prefer to leave this question to @boaks or @sbernard31

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I remember right, the idea was to move the turst-validation of the cert-chain out of californium/scandium to make it more flexible, e.g. use a tenant provided CA. Because scandium is currently too limited in the credential management, that's not possible in DTLS. For TLS using a more sophisticated TrustManager or X509ExtendedKeyManager should be the better approach.

I'm not really sure, where @AlexITC want to add the code for the tenant aware CA cert-chain validation. I guess, when this information is provided, we see, if this is a good idea or if there are better ones.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I implemented that using a custom Authorizer, as I'm not really familiar with Leshan Cluster, I can't say if it is ok to not serialize the certificate chain if we depend on it.

My understanding is that even if we don't serialize it, as the information comes from every request, that should be ok.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FMPOV:
We need to validate the trust either on handshake, or at least once after that.
One solution for an upper layer would be, to create a validated cache for such credentials and validate all receiving messages (not only the messages for the registration interface) against this cache, if it's already in, OK - trust, if not, try to validate and if trusted, add it to the cache on success.

@AlexITC
Did you try to use the java TrustManager or X509ExtendedTrustManager to implement your tenant aware trusting? If that was successful, we may consider to change scandium accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From this discussion, it seems that being able to introduce some custom validation logic in scandium would be a better solution. It would be properly managed by the DTLS layer for all handshakes, with or without a registration.
Here is a proposal: eclipse-californium/californium#602
Let me know what you think.

Copy link
Contributor

@AlexITC AlexITC Apr 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@boaks I used a custom microservice focused on that part.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

custom microservice

Smells like something potentially takes more time then the trust API assumes :-).

From my experience, the current credentials/trust APIs are intended to be used in a sync and Immediate way; they are not intended to delegated to a asynchronous, remote and delayed micro service. Such delays will tend to block the "limited number of threads" used by the systems. This may change, but I'm not aware, that such other solutions are ready.

Therefore the idea to just finish the handshake and delegate the trust verification to the next layer will enable that layer to decouple the "trust verification" from the threads in a asynchronous manner.

@msangoi
Your scandium PR is very welcome, but, (as I suspect for my self and therefore asked about the usage of the TrustManager), will raise follow-up issues.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right, sadly, that's the only way we could accomplish our goals with Leshan, until now, we haven't had issues due to these blocking calls.

peerIdentity = new X509CertPath(identity.getCertificates());
} else {
/* simplify distinguished name to CN= part */
peerIdentity = new X500Principal("CN=" + identity.getX509CommonName());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to create a separate method in X509Util to create the CN part.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@msangoi msangoi force-pushed the add-cert-chain-to-identity branch from 1c348c5 to 7569418 Compare March 29, 2018 07:59
@msangoi
Copy link
Contributor Author

msangoi commented Jun 7, 2018

I close this PR because it is now possible to introduce a custom certificate validation through the DtlsConnectorConfig (see eclipse-californium/californium#602).

@msangoi msangoi closed this Jun 7, 2018
@msangoi msangoi deleted the add-cert-chain-to-identity branch June 7, 2018 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants