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

Support Subject Alternative Names for SSL connections #952

Merged
merged 7 commits into from Oct 25, 2017

Conversation

Projects
None yet
5 participants
@davecramer
Member

davecramer commented Sep 20, 2017

Qualify the type of the alternative name with ALT_DNS_NAME to match libpq.

Previous implementation: #743

try {
// Check for Subject Alternative Names (see RFC 6125)
Collection subjectAltNames = serverCert.getSubjectAlternativeNames();

This comment has been minimized.

@sehrope

sehrope Sep 20, 2017

Contributor

If there is no SAN list in the cert then this would return null which would lead to an NPE. Need to add a ... !=null check and if null then skip SAN validation.

@codecov-io

This comment has been minimized.

codecov-io commented Sep 20, 2017

Codecov Report

Merging #952 into master will decrease coverage by <.01%.
The diff coverage is 41.66%.

@@             Coverage Diff             @@
##             master    #952      +/-   ##
===========================================
- Coverage      65.9%   65.9%   -0.01%     
- Complexity     3558    3570      +12     
===========================================
  Files           166     167       +1     
  Lines         15223   15292      +69     
  Branches       2458    2473      +15     
===========================================
+ Hits          10033   10078      +45     
- Misses         4023    4042      +19     
- Partials       1167    1172       +5
Integer type = (Integer) list.get(0);
// this mimics libpq check for ALT_DNS_NAME
if (type == ALT_DNS_NAME && san.equals(hostname)) {

This comment has been minimized.

@vlsi

vlsi Sep 20, 2017

Member

Would you please add null checks for type and san just in case?

while (sanIt.hasNext()) {
List list = (List) sanIt.next();
String san = ((String) list.get(1));

This comment has been minimized.

@vlsi

vlsi Sep 20, 2017

Member

Extra braces, and san/type should probably be declared in a type, san order as per getSubjectAlternativeNames documentation

Iterator sanIt = subjectAltNames.iterator();
while (sanIt.hasNext()) {

This comment has been minimized.

@vlsi

vlsi Sep 20, 2017

Member

foreach?

}
}
} catch (CertificateParsingException e) {
return false;

This comment has been minimized.

@vlsi

vlsi Sep 20, 2017

Member

Should the exception be logged? (e.g. log.warning)

@vlsi

This comment has been minimized.

Member

vlsi commented Sep 25, 2017

@davecramer , please add error handling

String san = (String) list.get(1);
// this mimics libpq check for ALT_DNS_NAME
if (type != null && san != null && type == ALT_DNS_NAME && san.equals(hostname)) {

This comment has been minimized.

@vlsi

vlsi Sep 25, 2017

Member

Please add .toLowerCase hostname normalization.
hostname.toLowerCase should be outside of a loop )

This comment has been minimized.

@jorsol

jorsol Sep 25, 2017

Contributor

IMO, an .equalsIgnoreCase(hostname) should be safer, or a .toLowerCase(Locale.ENGLISH) to san.

see http://www.i18nguy.com/unicode/turkish-i18n.html

This comment has been minimized.

@vlsi

vlsi Sep 25, 2017

Member

Thanks, makes sense. 👍 for equalsIgnoreCase

@jorsol

An enhanced for each can make the code more readable.

if (subjectAltNames != null) {
for (Iterator<List> sanit = subjectAltNames.iterator(); sanit.hasNext();) {

This comment has been minimized.

@jorsol

jorsol Sep 25, 2017

Contributor

An enhanced for each can make the code more readable:

Collection<List<?>> subjectAltNames = serverCert.getSubjectAlternativeNames();
if (subjectAltNames != null) {
  for (List<?> sanit : subjectAltNames) {
    Integer type = (Integer) sanit.get(0);
    String san = (String) sanit.get(1);

    // this mimics libpq check for ALT_DNS_NAME
    if (type != null && san != null && type == ALT_DNS_NAME && san.equalsIgnoreCase(hostname)) {
      return true;
    }
  }
}

@vlsi vlsi changed the title from rebased PR#743 subject alternative names to Support Subject Alternative Names for SSL connections Sep 25, 2017

vlsi added some commits Oct 25, 2017

@davecramer

This comment has been minimized.

Member

davecramer commented on c7aadc4 Oct 25, 2017

LGTM

@vlsi

vlsi approved these changes Oct 25, 2017

@vlsi vlsi added this to the 42.1.5 milestone Oct 25, 2017

@vlsi vlsi merged commit 2dcb91e into pgjdbc:master Oct 25, 2017

1 of 2 checks passed

codecov/project 65.9% (-0.01%) compared to 646a868
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@vlsi vlsi modified the milestones: 42.1.5, 42.2.0 Jan 8, 2018

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