You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2017/04/26 02:39:06 UTC
[kudu-CR] KUDU-1981 Kudu should run at hosts len(FQDN) > 64
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6734
Change subject: KUDU-1981 Kudu should run at hosts len(FQDN) > 64
......................................................................
KUDU-1981 Kudu should run at hosts len(FQDN) > 64
This is a fix for KUDU-1981: With security enabled, Kudu servers cannot
not start at machines with len(FQDN) > 64. Prior to this fix, the host
FQDN was put into the CN (common name) CSR/X509 field. Per RFC5280, the
CN field cannot strings longer than 64 characters long, and OpenSSL
enforces that limit as required.
The idea is to put FQDNs into the SAN X509v3 extension field as 'DNS'
fields. This allows to have names in the SAN which are even longer
than 255 characters. This patch returns back a part of the SAN-related
functionality implemented initially in cert_management.cc and then
removed since it was not used.
Added a couple of unit tests to cover the new functionality and to make
sure it's possible to set CN field of CSR to 64-chars length values and
have corresponding X509 certificate generated with no issues.
Change-Id: Ie142e76e9b2dcef3e07dd33d82b6758c746ced19
---
M src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/tls_context.cc
7 files changed, 175 insertions(+), 23 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/34/6734/1
--
To view, visit http://gerrit.cloudera.org:8080/6734
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie142e76e9b2dcef3e07dd33d82b6758c746ced19
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] KUDU-1981 Kudu should run at hosts len(FQDN) > 64
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6734
to look at the new patch set (#2).
Change subject: KUDU-1981 Kudu should run at hosts len(FQDN) > 64
......................................................................
KUDU-1981 Kudu should run at hosts len(FQDN) > 64
This is a fix for KUDU-1981: with security enabled, Kudu servers cannot
start at machines with len(FQDN) > 64. Prior to this fix, the host FQDN
was put into the CSR's CN (common name) field while generating
self-signed certificate for server RPC messenger. Per RFC5280, the CN
field cannot contain strings longer than 64 characters long, and it
seems OpenSSL enforces that limit as required.
The idea is to put FQDNs into the SAN X509v3 extension field as 'DNS'
fields. That makes it possible to have names in the SAN which are even
longer than 255 characters. This patch returns back a part of the
SAN-related functionality which had been implemented initially in
cert_management.cc and then removed since it was not used back then.
This patch also adds a couple of unit tests to cover the new
functionality and to make sure it's possible to set CN field of CSR to
64-chars length value and have corresponding X509 certificate generated
with no issues.
Change-Id: Ie142e76e9b2dcef3e07dd33d82b6758c746ced19
---
M src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/tls_context.cc
7 files changed, 178 insertions(+), 26 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/34/6734/2
--
To view, visit http://gerrit.cloudera.org:8080/6734
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie142e76e9b2dcef3e07dd33d82b6758c746ced19
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1981 Kudu should run at hosts len(FQDN) > 64
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged.
Change subject: KUDU-1981 Kudu should run at hosts len(FQDN) > 64
......................................................................
KUDU-1981 Kudu should run at hosts len(FQDN) > 64
This is a fix for KUDU-1981: with security enabled, Kudu servers cannot
start at machines with len(FQDN) > 64. Prior to this fix, the host FQDN
was put into the CSR's CN (common name) field while generating
self-signed certificate for server RPC messenger. Per RFC5280, the CN
field cannot contain strings longer than 64 characters long, and it
seems OpenSSL enforces that limit as required.
The idea is to put FQDNs into the SAN X509v3 extension field as 'DNS'
fields. That makes it possible to have names in the SAN which are even
longer than 255 characters. This patch returns back a part of the
SAN-related functionality which had been implemented initially in
cert_management.cc and then removed since it was not used back then.
This patch also adds a couple of unit tests to cover the new
functionality and to make sure it's possible to set CN field of CSR to
64-chars length value and have corresponding X509 certificate generated
with no issues.
Change-Id: Ie142e76e9b2dcef3e07dd33d82b6758c746ced19
Reviewed-on: http://gerrit.cloudera.org:8080/6734
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert <da...@apache.org>
---
M src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
M src/kudu/security/cert-test.cc
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
M src/kudu/security/tls_context.cc
10 files changed, 232 insertions(+), 74 deletions(-)
Approvals:
Dan Burkert: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/6734
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie142e76e9b2dcef3e07dd33d82b6758c746ced19
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1981 Kudu should run at hosts len(FQDN) > 64
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1981 Kudu should run at hosts len(FQDN) > 64
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6734/2/src/kudu/security/ca/cert_management-test.cc
File src/kudu/security/ca/cert_management-test.cc:
PS2, Line 179: // Verify that it's possible to set the common name field up to the maximum of
: // 64 characters while generating request and signing the certificate. The CN
: // (common name) field goes into the subject name of the result certificate
: // along with the UID (user id) field.
: T
not following the purpose of this test. it seems like we're redundantly testing OpenSSL here
http://gerrit.cloudera.org:8080/#/c/6734/2/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
Line 297: config->hostnames = { fqdn };
if we're now only using the SAN field, can we just get rid of the CN field entirely, rather than keeping the dead code path? (maybe I'm misunderstanding). Seems like we could kill off a number of lines of code.
--
To view, visit http://gerrit.cloudera.org:8080/6734
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie142e76e9b2dcef3e07dd33d82b6758c746ced19
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1981 Kudu should run at hosts len(FQDN) > 64
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1981 Kudu should run at hosts len(FQDN) > 64
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6734/3/src/kudu/security/cert.cc
File src/kudu/security/cert.cc:
Line 107: LOG(DFATAL) << "invalid DNS name is the SAN field";
should 'is' be 'in' here? It's a bit confusing as written. And having a return after LOG(DFATAL) is a noop, right?
http://gerrit.cloudera.org:8080/#/c/6734/3/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
Line 297: config->hostnames = { fqdn };
Looks like we would never set more than a single hostname for the FQDN? Should we simplify the interface to only take a single string instead of a vector?
--
To view, visit http://gerrit.cloudera.org:8080/6734
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie142e76e9b2dcef3e07dd33d82b6758c746ced19
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1981 Kudu should run at hosts len(FQDN) > 64
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KUDU-1981 Kudu should run at hosts len(FQDN) > 64
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6734/3/src/kudu/security/cert.cc
File src/kudu/security/cert.cc:
Line 107: LOG(DFATAL) << "invalid DNS name is the SAN field";
> should 'is' be 'in' here? It's a bit confusing as written. And having a r
Done
http://gerrit.cloudera.org:8080/#/c/6734/3/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
Line 297: config->hostnames = { fqdn };
> Looks like we would never set more than a single hostname for the FQDN? Sh
Done
--
To view, visit http://gerrit.cloudera.org:8080/6734
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie142e76e9b2dcef3e07dd33d82b6758c746ced19
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1981 Kudu should run at hosts len(FQDN) > 64
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6734
to look at the new patch set (#4).
Change subject: KUDU-1981 Kudu should run at hosts len(FQDN) > 64
......................................................................
KUDU-1981 Kudu should run at hosts len(FQDN) > 64
This is a fix for KUDU-1981: with security enabled, Kudu servers cannot
start at machines with len(FQDN) > 64. Prior to this fix, the host FQDN
was put into the CSR's CN (common name) field while generating
self-signed certificate for server RPC messenger. Per RFC5280, the CN
field cannot contain strings longer than 64 characters long, and it
seems OpenSSL enforces that limit as required.
The idea is to put FQDNs into the SAN X509v3 extension field as 'DNS'
fields. That makes it possible to have names in the SAN which are even
longer than 255 characters. This patch returns back a part of the
SAN-related functionality which had been implemented initially in
cert_management.cc and then removed since it was not used back then.
This patch also adds a couple of unit tests to cover the new
functionality and to make sure it's possible to set CN field of CSR to
64-chars length value and have corresponding X509 certificate generated
with no issues.
Change-Id: Ie142e76e9b2dcef3e07dd33d82b6758c746ced19
---
M src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
M src/kudu/security/cert-test.cc
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
M src/kudu/security/tls_context.cc
10 files changed, 232 insertions(+), 74 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/34/6734/4
--
To view, visit http://gerrit.cloudera.org:8080/6734
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie142e76e9b2dcef3e07dd33d82b6758c746ced19
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1981 Kudu should run at hosts len(FQDN) > 64
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6734
to look at the new patch set (#3).
Change subject: KUDU-1981 Kudu should run at hosts len(FQDN) > 64
......................................................................
KUDU-1981 Kudu should run at hosts len(FQDN) > 64
This is a fix for KUDU-1981: with security enabled, Kudu servers cannot
start at machines with len(FQDN) > 64. Prior to this fix, the host FQDN
was put into the CSR's CN (common name) field while generating
self-signed certificate for server RPC messenger. Per RFC5280, the CN
field cannot contain strings longer than 64 characters long, and it
seems OpenSSL enforces that limit as required.
The idea is to put FQDNs into the SAN X509v3 extension field as 'DNS'
fields. That makes it possible to have names in the SAN which are even
longer than 255 characters. This patch returns back a part of the
SAN-related functionality which had been implemented initially in
cert_management.cc and then removed since it was not used back then.
This patch also adds a couple of unit tests to cover the new
functionality and to make sure it's possible to set CN field of CSR to
64-chars length value and have corresponding X509 certificate generated
with no issues.
Change-Id: Ie142e76e9b2dcef3e07dd33d82b6758c746ced19
---
M src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/tls_context.cc
7 files changed, 201 insertions(+), 69 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/34/6734/3
--
To view, visit http://gerrit.cloudera.org:8080/6734
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie142e76e9b2dcef3e07dd33d82b6758c746ced19
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1981 Kudu should run at hosts len(FQDN) > 64
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1981 Kudu should run at hosts len(FQDN) > 64
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/6734
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie142e76e9b2dcef3e07dd33d82b6758c746ced19
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1981 Kudu should run at hosts len(FQDN) > 64
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KUDU-1981 Kudu should run at hosts len(FQDN) > 64
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6734/2/src/kudu/security/ca/cert_management-test.cc
File src/kudu/security/ca/cert_management-test.cc:
PS2, Line 179: // Verify that it's possible to set the common name field up to the maximum of
: // 64 characters while generating request and signing the certificate. The CN
: // (common name) field goes into the subject name of the result certificate
: // along with the UID (user id) field.
: T
> not following the purpose of this test. it seems like we're redundantly tes
Just wanted to make it explicit via adding a small test.
From other other side, not much functionality is checked. I'll remove it.
http://gerrit.cloudera.org:8080/#/c/6734/2/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
Line 297: config->hostnames = { fqdn };
> if we're now only using the SAN field, can we just get rid of the CN field
That's a good idea -- at least we can get rid of that field for non-CA certs. For CA certs DNS names do not have much sense and I'm not sure whether we want use other available types of fields (URI, e-mail, IP Addresses) in case of CA certs.
--
To view, visit http://gerrit.cloudera.org:8080/6734
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie142e76e9b2dcef3e07dd33d82b6758c746ced19
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes