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.

Sign up for GitHub

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

Jump to bottom

FingerprintTrustManagerFactory javadoc note cryptic #5173

Closed
CodingFabian opened this issue Apr 25, 2016 · 7 comments
Closed

FingerprintTrustManagerFactory javadoc note cryptic #5173

CodingFabian opened this issue Apr 25, 2016 · 7 comments
Assignees
@normanmaurer
Labels
cleanup documentation
Milestone
4.0.37.Final

Comments

@CodingFabian
Copy link
Contributor

the javadoc says:

"Never use this {@link TrustManagerFactory} in production unless you are sure exactly what you are doing with it."

I was actually sure that in order to prevent MITM attacks, I want to pin the serverside certificates to a list of well known fingerprints. I do not want to accept other certificates.

So I think I know what I am doing, but this note made me feel uneasy. Why is it formulated so strongly? Why is it not explaining what "this TrustmanagerFactory" actually does?
I am currently trying to figure out if there is an unintentional side effect, but can't find it :-)
@trustin
Copy link
Member

trustin commented Apr 25, 2016

It only checks the fingerprint. It does not traverse the certificate chain at all. Theoretically, there's a chance where an attacker can forge a certificate with identical fingerprint, although it's not going to be very easy.

I agree that it needs better documentation though. Would you be interested in sending us a PR?

@CodingFabian
Copy link
Contributor Author

Ah ok, it makes sense to point that out. Will make a PR to improve docs.

@trustin
Copy link
Member

trustin commented Apr 25, 2016

Thanks a lot, @CodingFabian !

@normanmaurer
Copy link
Member

@CodingFabian any progress here ?

@CodingFabian
Copy link
Contributor Author

woops, forgot. coming

@normanmaurer
Copy link
Member

@CodingFabian 😍

@CodingFabian CodingFabian mentioned this issue May 2, 2016
Closed
CodingFabian added a commit to CodingFabian/netty that referenced this issue May 2, 2016
@CodingFabian
[netty#5173] Rewrite misleading Note in FingerprintTrustManagerFactor…
08c74e9 
…y javadoc

Motivation:

The current note reads as if this class is dangerous and advises the reader to "understand what this class does".

Modifications:

Rewrite the Javadoc note to describe what fingerprint checks are and what problems remain.

Result:

Clearer description which no longer causes the impression this class is dangerous.
@normanmaurer normanmaurer self-assigned this May 3, 2016
@normanmaurer normanmaurer added this to the 4.0.37.Final milestone May 3, 2016
@normanmaurer
Copy link
Member

Fixed by #5195

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@normanmaurer normanmaurer

Labels
cleanup documentation
Projects
None yet
Milestone
4.0.37.Final
Development

No branches or pull requests
3 participants
@trustin @CodingFabian @normanmaurer