Skip to content

Fix OpenSslCertificateException error code validation#6581

Closed
normanmaurer wants to merge 1 commit into4.1from
openssl_exception_validate
Closed

Fix OpenSslCertificateException error code validation#6581
normanmaurer wants to merge 1 commit into4.1from
openssl_exception_validate

Conversation

@normanmaurer
Copy link
Copy Markdown
Member

@normanmaurer normanmaurer commented Mar 29, 2017

Motivation:

In OpenSslCertificateException we tried to validate the supplied error code but did not correctly account for all different valid error codes and so threw an IllegalArgumentException.

Modifications:

  • Fix validation
  • Add unit tests

Result:

Validation of error code works as expected.

@normanmaurer normanmaurer self-assigned this Mar 29, 2017
@normanmaurer normanmaurer added this to the 4.0.46.Final milestone Mar 29, 2017
normanmaurer
normanmaurer commented Mar 29, 2017
View reviewed changes
handler/src/main/java/io/netty/handler/ssl/OpenSslCertificateException.java Outdated

// Package-private for testing
static boolean isValid(int errorCode) {
return errorCode == CertificateVerifier.X509_V_OK ||
Copy link
Copy Markdown
Member Author

@normanmaurer normanmaurer Mar 29, 2017

Choose a reason for hiding this comment

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

can't use a switch here :(

Copy link
Copy Markdown
Contributor

@fenik17 fenik17 Mar 29, 2017

Choose a reason for hiding this comment

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

HashSet?

normanmaurer
normanmaurer commented Mar 29, 2017
View reviewed changes
handler/src/main/java/io/netty/handler/ssl/OpenSslCertificateException.java Outdated
// Package-private for testing
static boolean isValid(int errorCode) {
return errorCode == CertificateVerifier.X509_V_OK ||
errorCode == CertificateVerifier.X509_V_ERR_UNSPECIFIED ||
Copy link
Copy Markdown
Member Author

@normanmaurer normanmaurer Mar 29, 2017

Choose a reason for hiding this comment

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

@Scottmitch I am not sure if this should life here or if we should better move it to tcnative. WDYT ?

fenik17
fenik17 reviewed Mar 29, 2017
View reviewed changes
handler/src/main/java/io/netty/handler/ssl/OpenSslCertificateException.java Outdated
errorCode == CertificateVerifier.X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT ||
errorCode == CertificateVerifier.X509_V_ERR_UNABLE_TO_GET_CRL ||
errorCode == CertificateVerifier.X509_V_ERR_UNABLE_TO_DECRYPT_CERT_SIGNATURE ||
errorCode == CertificateVerifier. X509_V_ERR_UNABLE_TO_DECRYPT_CRL_SIGNATURE ||
Copy link
Copy Markdown
Contributor

@fenik17 fenik17 Mar 29, 2017

Choose a reason for hiding this comment

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

nit: out of place a whitespace

fenik17
fenik17 reviewed Mar 29, 2017
View reviewed changes
handler/src/main/java/io/netty/handler/ssl/OpenSslCertificateException.java Outdated
errorCode == CertificateVerifier.X509_V_ERR_INVALID_NON_CA ||
errorCode == CertificateVerifier.X509_V_ERR_PROXY_PATH_LENGTH_EXCEEDED ||
errorCode == CertificateVerifier.X509_V_ERR_KEYUSAGE_NO_DIGITAL_SIGNATURE ||
errorCode == CertificateVerifier. X509_V_ERR_PROXY_CERTIFICATES_NOT_ALLOWED ||
Copy link
Copy Markdown
Contributor

@fenik17 fenik17 Mar 29, 2017

Choose a reason for hiding this comment

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

nit: out of place a whitespace

@normanmaurer normanmaurer force-pushed the openssl_exception_validate branch from 31b4871 to 75b27e9 Compare March 29, 2017 11:29
@normanmaurer
Copy link
Copy Markdown
Member Author

normanmaurer commented Mar 29, 2017

@fenik17 PTAL again

fenik17
fenik17 reviewed Mar 29, 2017
View reviewed changes
handler/src/main/java/io/netty/handler/ssl/OpenSslCertificateException.java Outdated
Copy link
Copy Markdown
Contributor

@fenik17 fenik17 Mar 29, 2017

Choose a reason for hiding this comment

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

needless whitespace still here )

perf002

Copy link
Copy Markdown
Member Author

@normanmaurer normanmaurer Mar 29, 2017

Choose a reason for hiding this comment

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

fixed

fenik17
fenik17 reviewed Mar 29, 2017
View reviewed changes
handler/src/main/java/io/netty/handler/ssl/OpenSslCertificateException.java Outdated
Copy link
Copy Markdown
Contributor

@fenik17 fenik17 Mar 29, 2017

Choose a reason for hiding this comment

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

ditto

Copy link
Copy Markdown
Member Author

@normanmaurer normanmaurer Mar 29, 2017

Choose a reason for hiding this comment

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

fixed

@normanmaurer normanmaurer force-pushed the openssl_exception_validate branch from 75b27e9 to b301f9d Compare March 29, 2017 12:19
fenik17
fenik17 reviewed Mar 29, 2017
View reviewed changes
handler/src/main/java/io/netty/handler/ssl/OpenSslCertificateException.java Outdated
Copy link
Copy Markdown
Contributor

@fenik17 fenik17 Mar 29, 2017

Choose a reason for hiding this comment

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

in manmaster 404 for this link..

Copy link
Copy Markdown
Member Author

@normanmaurer normanmaurer Mar 29, 2017

Choose a reason for hiding this comment

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

fixed link

@normanmaurer normanmaurer force-pushed the openssl_exception_validate branch from b301f9d to 98936ff Compare March 29, 2017 12:38
@Scottmitch
Copy link
Copy Markdown
Member

Scottmitch commented Mar 29, 2017

@normanmaurer - regarding #6581 (comment) ... it seems less likely we will miss updating the validation logic if the validation code lives in tcnative ... and there is a clear relationship between the values and the validation code.

@normanmaurer
Copy link
Copy Markdown
Member Author

normanmaurer commented Mar 29, 2017

@Scottmitch yep sounds good to me as well. Let me move it to tcnative

@normanmaurer normanmaurer force-pushed the openssl_exception_validate branch from 98936ff to d4c38a7 Compare March 30, 2017 13:26
@normanmaurer
Copy link
Copy Markdown
Member Author

normanmaurer commented Mar 30, 2017

This depends on netty/netty-tcnative#260

Scottmitch
Scottmitch requested changes Mar 31, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@Scottmitch Scottmitch left a comment

Choose a reason for hiding this comment

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

1 small change then lgtm

handler/src/test/java/io/netty/handler/ssl/OpenSslCertificateExceptionTest.java Outdated
do {
errorCode++;
assert errorCode < Integer.MAX_VALUE;
} while (CertificateVerifier.isValid(errorCode));
Copy link
Copy Markdown
Member

@Scottmitch Scottmitch Mar 31, 2017

Choose a reason for hiding this comment

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

simplify this test just like we did in tcnative.

@normanmaurer
Fix OpenSslCertificateException error code validation
c0db3df 
Motivation:

In OpenSslCertificateException we tried to validate the supplied error code but did not correctly account for all different valid error codes and so threw an IllegalArgumentException.

Modifications:

- Fix validation by updating to latest netty-tcnative and use CertificateVerifier.isValid
- Add unit tests

Result:

Validation of error code works as expected.
@normanmaurer
Copy link
Copy Markdown
Member Author

normanmaurer commented Mar 31, 2017

Addressed

@normanmaurer normanmaurer force-pushed the openssl_exception_validate branch from d4c38a7 to c0db3df Compare March 31, 2017 08:15
@Scottmitch
Copy link
Copy Markdown
Member

Scottmitch commented Apr 3, 2017

4.1 (4bcfa07) 4.0 (82661bc)

@Scottmitch Scottmitch closed this Apr 3, 2017
@Scottmitch Scottmitch deleted the openssl_exception_validate branch April 3, 2017 18:12
@Scottmitch Scottmitch mentioned this pull request Apr 3, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Scottmitch Scottmitch Scottmitch approved these changes

@nmittler nmittler Awaiting requested review from nmittler

+1 more reviewer

@fenik17 fenik17 fenik17 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@normanmaurer normanmaurer

Labels

defect

Projects

None yet

Milestone

4.0.46.Final

Development

Successfully merging this pull request may close these issues.

3 participants

@normanmaurer @Scottmitch @fenik17