You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2017/02/17 07:00:43 UTC
[kudu-CR] [security] security-flags
Hello Adar Dembo, Todd Lipcon, Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/6052
to review the following change.
Change subject: [security] security-flags
......................................................................
[security] security-flags
This commit introduces, removes, and renames security flags in an effort
to make the flags more consistent, more understandable, and easier to
use from the command line. Affected flags:
--rpc_authentication
This is a new flag which will apply to Kudu servers and the kudu
command line tool which will allow operators to configure a policy
for authentication of RPC connections. The possible values are
'enabled', 'disabled', and 'required'. Three states are necessary
(as opposed to just 'disabled' and 'required') in order to provide a
graceful upgrade path for clusters from unsecured to secured.
'enabled' is the default. A follow up commit will hook this flag
into the RPC system to ensure that authentication is enforced as
necessary.
--rpc_encryption
This is a new flag which will apply to Kudu servers and the 'kudu'
command line tool which allows operators to configure a policy for
encryption on RPC connections. This is a tristate flag for the same
reasons as outlined in --rpc_authentication. A follow up commit will
hook this flag into the RPC system to ensure that encryption is
enforced as necessary.
--server_require_kerberos
This flag has been removed, and in it's place the --keytab and
--rpc_authentication=required flags are provided. --keytab is used
to enable Kerberos authentication on a server, and
--rpc_authentication=required is used to ensure that all RPCs use
authentication.
--rpc_cert
--rpc_ssl_server_certificate
--rpc_cert is replacing --rpc_ssl_server_certificate. The latter has
a few issues. 1) It's not strictly a server flag, it also applies to
the kudu CLI tool. 2) It's really long, and the length doesn't add
useful description or specificity. 3) The short form (cert instead
of certificate) is common in database CLI configs [1], [2], [3].
--rpc_key
--rpc_ssl_private_key
--rpc_key is replacing --rpc_ssl_private_key. Same reasons as
--rpc_cert.
--rpc_ca_cert
--rpc_ssl_certificate_authority
--rpc_key is replacing --rpc_ssl_private_key. Same reasons as
--rpc_cert.
--webserver_cert
--webserver_certificate_file
--webserver_cert is replacing --webserver_certificate file to stay
consistent with the analogous RPC flag.
--webserver_key
--webserver_private_key_file
--webserver_key is replacing --webserver_private_key_file to stay
consistent with the analogous RPC flag.
--webserver_key_password_cmd
--webserver_private_key_password_cmd
--webserver_key_password_cmd is replacing
--webserver_private_key_password_cmd to stay consistent with
--webserver_key.
[1] https://rethinkdb.com/docs/security/#using-tls
[2] https://www.cockroachlabs.com/docs/create-security-certificates.html#flags
[3] https://docs.mongodb.com/manual/tutorial/configure-ssl/
I looked at Postgres and MySQL as well, but both of those look in the
data directory for the cert and key files, so they don't have individual
CLI arguments. However, they both commonly refer to certificates as
certs in the documentation.
Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
---
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/sasl_common.cc
M src/kudu/server/webserver_options.cc
8 files changed, 118 insertions(+), 62 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/6052/1
--
To view, visit http://gerrit.cloudera.org:8080/6052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [security] security-flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6052
to look at the new patch set (#4).
Change subject: [security] security-flags
......................................................................
[security] security-flags
This commit introduces, removes, and renames security flags in an effort
to make the flags more consistent, more understandable, and easier to
use from the command line. Affected flags:
--rpc_authentication
This is a new flag which will apply to Kudu servers and the kudu
command line tool which will allow operators to configure a policy
for authentication of RPC connections. The possible values are
'enabled', 'disabled', and 'required'. Three states are necessary
(as opposed to just 'disabled' and 'required') in order to provide a
graceful upgrade path for clusters from unsecured to secured.
'enabled' is the default. A follow up commit will hook this flag
into the RPC system to ensure that authentication is enforced as
necessary.
--rpc_encryption
This is a new flag which will apply to Kudu servers and the 'kudu'
command line tool which allows operators to configure a policy for
encryption on RPC connections. This is a tristate flag for the same
reasons as outlined in --rpc_authentication. A follow up commit will
hook this flag into the RPC system to ensure that encryption is
enforced as necessary.
--server_require_kerberos
This flag has been removed, and in it's place the --keytab and
--rpc_authentication=required flags are provided. --keytab is used
to enable Kerberos authentication on a server, and
--rpc_authentication=required is used to ensure that all RPCs use
authentication.
--rpc_certificate_file
--rpc_ssl_server_certificate
--rpc_certificate_file is replacing --rpc_ssl_server_certificate.
The latter has a few issues. 1) It's not strictly a server flag, it
also applies to the kudu CLI tool. 3) It's not consistent with the
similar --webserver_certificate_file flag.
--rpc_private_key_file
--rpc_ssl_private_key
--rpc_private_key_file is replacing --rpc_ssl_private_key. Same
reasons as --rpc_cert.
--rpc_ca_certificate_file
--rpc_ssl_certificate_authority
--rpc_ca_certificate_file is replacing
--rpc_ssl_certificate_authority. Same reasons as --rpc_cert.
Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
---
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/sasl_common.cc
7 files changed, 108 insertions(+), 49 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/6052/4
--
To view, visit http://gerrit.cloudera.org:8080/6052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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] [security] security-flags
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [security] security-flags
......................................................................
Patch Set 2:
(2 comments)
Just commenting on the overall approach; didn't look at the implementation.
http://gerrit.cloudera.org:8080/#/c/6052/2//COMMIT_MSG
Commit Message:
PS2, Line 43: 2) It's really long, and the length doesn't add
: useful description or specificity. 3) The short form (cert instead
: of certificate) is common in database CLI configs [1], [2], [3].
I mentioned this last night in #kudu-security on Slack, but I'll repeat it here too; I'm curious whether anyone else feels the way I do.
While I appreciate that consistency w.r.t. other databases is a useful thing, I'm concerned that the gain is outweighed by the loss of information in shortening some of these flag names. I for one appreciate the "_file" suffix in the webserver-related flags, which made it very clear that the flag value should be a path to a file. Likewise, "_key_" is more ambiguous than "_private_key_"; it's not immediately clear whether a private or public key is being requested.
PS2, Line 54: --rpc_key is replacing --rpc_ssl_private_key.
Copy/pasta.
--
To view, visit http://gerrit.cloudera.org:8080/6052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [security] security-flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6052
to look at the new patch set (#6).
Change subject: [security] security-flags
......................................................................
[security] security-flags
This commit introduces, removes, and renames security flags in an effort
to make the flags more consistent, more understandable, and easier to
use from the command line. Affected flags:
--rpc_authentication
This is a new flag which will apply to Kudu servers and the kudu
command line tool which will allow operators to configure a policy
for authentication of RPC connections. The possible values are
'enabled', 'disabled', and 'required'. Three states are necessary
(as opposed to just 'disabled' and 'required') in order to provide a
graceful upgrade path for clusters from unsecured to secured.
'enabled' is the default. A follow up commit will hook this flag
into the RPC system to ensure that authentication is enforced as
necessary.
--rpc_encryption
This is a new flag which will apply to Kudu servers and the 'kudu'
command line tool which allows operators to configure a policy for
encryption on RPC connections. This is a tristate flag for the same
reasons as outlined in --rpc_authentication. A follow up commit will
hook this flag into the RPC system to ensure that encryption is
enforced as necessary.
--server_require_kerberos
This flag has been removed, and in it's place the --keytab and
--rpc_authentication=required flags are provided. --keytab is used
to enable Kerberos authentication on a server, and
--rpc_authentication=required is used to ensure that all RPCs use
authentication.
--rpc_certificate_file
--rpc_ssl_server_certificate
--rpc_certificate_file is replacing --rpc_ssl_server_certificate.
The latter has a few issues. 1) It's not strictly a server flag, it
also applies to the kudu CLI tool. 3) It's not consistent with the
similar --webserver_certificate_file flag.
--rpc_private_key_file
--rpc_ssl_private_key
--rpc_private_key_file is replacing --rpc_ssl_private_key. Same
reasons as --rpc_cert.
--rpc_ca_certificate_file
--rpc_ssl_certificate_authority
--rpc_ca_certificate_file is replacing
--rpc_ssl_certificate_authority. Same reasons as --rpc_cert.
Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
---
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/sasl_common.cc
7 files changed, 123 insertions(+), 50 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/6052/6
--
To view, visit http://gerrit.cloudera.org:8080/6052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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] [security] security-flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Todd Lipcon, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6052
to look at the new patch set (#2).
Change subject: [security] security-flags
......................................................................
[security] security-flags
This commit introduces, removes, and renames security flags in an effort
to make the flags more consistent, more understandable, and easier to
use from the command line. Affected flags:
--rpc_authentication
This is a new flag which will apply to Kudu servers and the kudu
command line tool which will allow operators to configure a policy
for authentication of RPC connections. The possible values are
'enabled', 'disabled', and 'required'. Three states are necessary
(as opposed to just 'disabled' and 'required') in order to provide a
graceful upgrade path for clusters from unsecured to secured.
'enabled' is the default. A follow up commit will hook this flag
into the RPC system to ensure that authentication is enforced as
necessary.
--rpc_encryption
This is a new flag which will apply to Kudu servers and the 'kudu'
command line tool which allows operators to configure a policy for
encryption on RPC connections. This is a tristate flag for the same
reasons as outlined in --rpc_authentication. A follow up commit will
hook this flag into the RPC system to ensure that encryption is
enforced as necessary.
--server_require_kerberos
This flag has been removed, and in it's place the --keytab and
--rpc_authentication=required flags are provided. --keytab is used
to enable Kerberos authentication on a server, and
--rpc_authentication=required is used to ensure that all RPCs use
authentication.
--rpc_cert
--rpc_ssl_server_certificate
--rpc_cert is replacing --rpc_ssl_server_certificate. The latter has
a few issues. 1) It's not strictly a server flag, it also applies to
the kudu CLI tool. 2) It's really long, and the length doesn't add
useful description or specificity. 3) The short form (cert instead
of certificate) is common in database CLI configs [1], [2], [3].
--rpc_key
--rpc_ssl_private_key
--rpc_key is replacing --rpc_ssl_private_key. Same reasons as
--rpc_cert.
--rpc_ca_cert
--rpc_ssl_certificate_authority
--rpc_key is replacing --rpc_ssl_private_key. Same reasons as
--rpc_cert.
--webserver_cert
--webserver_certificate_file
--webserver_cert is replacing --webserver_certificate file to stay
consistent with the analogous RPC flag.
--webserver_key
--webserver_private_key_file
--webserver_key is replacing --webserver_private_key_file to stay
consistent with the analogous RPC flag.
--webserver_key_password_cmd
--webserver_private_key_password_cmd
--webserver_key_password_cmd is replacing
--webserver_private_key_password_cmd to stay consistent with
--webserver_key.
[1] https://rethinkdb.com/docs/security/#using-tls
[2] https://www.cockroachlabs.com/docs/create-security-certificates.html#flags
[3] https://docs.mongodb.com/manual/tutorial/configure-ssl/
I looked at Postgres and MySQL as well, but both of those look in the
data directory for the cert and key files, so they don't have individual
CLI arguments. However, they both commonly refer to certificates as
certs in the documentation.
Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
---
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/sasl_common.cc
M src/kudu/server/webserver_options.cc
8 files changed, 119 insertions(+), 63 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/6052/2
--
To view, visit http://gerrit.cloudera.org:8080/6052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [security] security-flags
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [security] security-flags
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6052/4/src/kudu/rpc/negotiation.cc
File src/kudu/rpc/negotiation.cc:
Line 206: if (FLAGS_keytab.empty()) {
shouldn't this be checking whether the authentication = 'required'? for 'enabled', I'd think we would enable both plain and gssapi
--
To view, visit http://gerrit.cloudera.org:8080/6052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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] [security] security-flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6052
to look at the new patch set (#5).
Change subject: [security] security-flags
......................................................................
[security] security-flags
This commit introduces, removes, and renames security flags in an effort
to make the flags more consistent, more understandable, and easier to
use from the command line. Affected flags:
--rpc_authentication
This is a new flag which will apply to Kudu servers and the kudu
command line tool which will allow operators to configure a policy
for authentication of RPC connections. The possible values are
'enabled', 'disabled', and 'required'. Three states are necessary
(as opposed to just 'disabled' and 'required') in order to provide a
graceful upgrade path for clusters from unsecured to secured.
'enabled' is the default. A follow up commit will hook this flag
into the RPC system to ensure that authentication is enforced as
necessary.
--rpc_encryption
This is a new flag which will apply to Kudu servers and the 'kudu'
command line tool which allows operators to configure a policy for
encryption on RPC connections. This is a tristate flag for the same
reasons as outlined in --rpc_authentication. A follow up commit will
hook this flag into the RPC system to ensure that encryption is
enforced as necessary.
--server_require_kerberos
This flag has been removed, and in it's place the --keytab and
--rpc_authentication=required flags are provided. --keytab is used
to enable Kerberos authentication on a server, and
--rpc_authentication=required is used to ensure that all RPCs use
authentication.
--rpc_certificate_file
--rpc_ssl_server_certificate
--rpc_certificate_file is replacing --rpc_ssl_server_certificate.
The latter has a few issues. 1) It's not strictly a server flag, it
also applies to the kudu CLI tool. 3) It's not consistent with the
similar --webserver_certificate_file flag.
--rpc_private_key_file
--rpc_ssl_private_key
--rpc_private_key_file is replacing --rpc_ssl_private_key. Same
reasons as --rpc_cert.
--rpc_ca_certificate_file
--rpc_ssl_certificate_authority
--rpc_ca_certificate_file is replacing
--rpc_ssl_certificate_authority. Same reasons as --rpc_cert.
Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
---
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/sasl_common.cc
7 files changed, 124 insertions(+), 50 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/6052/5
--
To view, visit http://gerrit.cloudera.org:8080/6052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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] [security] security-flags
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [security] security-flags
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6052/4/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:
Line 62:
The new tri-state flags should be tagged as stable.
--
To view, visit http://gerrit.cloudera.org:8080/6052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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] [security] security-flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [security] security-flags
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6052/2//COMMIT_MSG
Commit Message:
PS2, Line 54: --rpc_ca_cert is replacing --rpc_ssl_certific
> Copy/pasta.
Done
--
To view, visit http://gerrit.cloudera.org:8080/6052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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] [security] security-flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [security] security-flags
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6052/2//COMMIT_MSG
Commit Message:
Line 58: --webserver_certificate_file
> this and the below two were chosen for compatibility with Impala. Obviously
Alright, another option is to leave the webserver options and change the rpc options to match:
--rpc_certificate_file
--rpc_ca_certificate_file
--rpc_private_key_file
I don't like this as much as the current proposal, but we're running out of time.
--
To view, visit http://gerrit.cloudera.org:8080/6052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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] [security] security-flags
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [security] security-flags
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/6052/4//COMMIT_MSG
Commit Message:
PS4, Line 43: 3)
Nit: #2
http://gerrit.cloudera.org:8080/#/c/6052/4/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:
Line 164: if (boost::iequals(FLAGS_rpc_authentication, "required")) {
Would be nice to do this validation in a per-gflag validator. Better experience as it fails faster.
PS4, Line 198: else if (!FLAGS_rpc_certificate_file.empty() ||
: !FLAGS_rpc_private_key_file.empty() ||
: !FLAGS_rpc_ca_certificate_file.empty()) {
: return Status::InvalidArgument("--rpc_certificate_file, --rpc_private_key_file, and "
: "--rpc_ca_certificate_file flags must be set as a group");
: }
Likewise with this.
--
To view, visit http://gerrit.cloudera.org:8080/6052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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] [security] security-flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [security] security-flags
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6052/2//COMMIT_MSG
Commit Message:
PS2, Line 20: A follow up commit will hook this flag
: into the RPC system to ensure that authentication is enforced as
: necessary.
> keep in mind that clients don't have the ability to configure flags, so we'
Yep.
PS2, Line 42: 1) It's not strictly a server flag, it also applies to
: the kudu CLI tool.
> how does it apply to the CLI tool? Given that we don't currently support cl
> I think that we should probably remove (or mark experimental/hidden) all of these flags until we officially support and test the BYOPKI option)
They are marked experimental. You are correct that BYOPKI is the only reason they exist, and in that case they are needed by both the servers and client. This is outlined in a comment in messenger.cc.
--
To view, visit http://gerrit.cloudera.org:8080/6052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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] [security] security-flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [security] security-flags
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6052/2//COMMIT_MSG
Commit Message:
PS2, Line 43: 2) It's really long, and the length doesn't add
: useful description or specificity. 3) The short form (cert instead
: of certificate) is common in database CLI configs [1], [2], [3].
> yea, I agree that 'rpc_key' isn't very descriptive. At the least I'd expect
Done
Line 58: --webserver_certificate_file
> Sure, I also don't really care too much about the RPC settings for now. Sam
Done
--
To view, visit http://gerrit.cloudera.org:8080/6052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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] [security] security-flags
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: [security] security-flags
......................................................................
[security] security-flags
This commit introduces, removes, and renames security flags in an effort
to make the flags more consistent, more understandable, and easier to
use from the command line. Affected flags:
--rpc_authentication
This is a new flag which will apply to Kudu servers and the kudu
command line tool which will allow operators to configure a policy
for authentication of RPC connections. The possible values are
'enabled', 'disabled', and 'required'. Three states are necessary
(as opposed to just 'disabled' and 'required') in order to provide a
graceful upgrade path for clusters from unsecured to secured.
'enabled' is the default. A follow up commit will hook this flag
into the RPC system to ensure that authentication is enforced as
necessary.
--rpc_encryption
This is a new flag which will apply to Kudu servers and the 'kudu'
command line tool which allows operators to configure a policy for
encryption on RPC connections. This is a tristate flag for the same
reasons as outlined in --rpc_authentication. A follow up commit will
hook this flag into the RPC system to ensure that encryption is
enforced as necessary.
--server_require_kerberos
This flag has been removed, and in it's place the --keytab and
--rpc_authentication=required flags are provided. --keytab is used
to enable Kerberos authentication on a server, and
--rpc_authentication=required is used to ensure that all RPCs use
authentication.
--rpc_certificate_file
--rpc_ssl_server_certificate
--rpc_certificate_file is replacing --rpc_ssl_server_certificate.
The latter has a few issues. 1) It's not strictly a server flag, it
also applies to the kudu CLI tool. 3) It's not consistent with the
similar --webserver_certificate_file flag.
--rpc_private_key_file
--rpc_ssl_private_key
--rpc_private_key_file is replacing --rpc_ssl_private_key. Same
reasons as --rpc_cert.
--rpc_ca_certificate_file
--rpc_ssl_certificate_authority
--rpc_ca_certificate_file is replacing
--rpc_ssl_certificate_authority. Same reasons as --rpc_cert.
Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
Reviewed-on: http://gerrit.cloudera.org:8080/6052
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/sasl_common.cc
7 files changed, 123 insertions(+), 50 deletions(-)
Approvals:
Todd Lipcon: Looks good to me, approved
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/6052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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] [security] security-flags
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [security] security-flags
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/6052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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] [security] security-flags
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [security] security-flags
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6052/2//COMMIT_MSG
Commit Message:
Line 58: --webserver_certificate_file
> My vote: consistency with Impala is a good thing, as we have quite a few us
Sure, I also don't really care too much about the RPC settings for now. Same logic as Adar - we can always change them later, since they're experimental/untested/unused.
--
To view, visit http://gerrit.cloudera.org:8080/6052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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] [security] security-flags
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [security] security-flags
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/6052/4//COMMIT_MSG
Commit Message:
PS4, Line 7:
nit: an extra space
Also, consider updating the description to be have some information on the changelist. E.g., consider 'revised security-flags' or something like that.
http://gerrit.cloudera.org:8080/#/c/6052/4/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:
PS4, Line 84: cert, "",
--rpc_certificate_file ?
http://gerrit.cloudera.org:8080/#/c/6052/4/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:
PS4, Line 321: if (!FLAGS_keytab.empty()) kudu::security::KerberosReinitLock()->ReadLock();
: int rc = call();
: if (!FLAGS_keytab.empty
nit: any chance of FLAG_keytab to change in between (test, etc.)? If so, consider sampling value of the flag into a local variable and use its value for lock/unlock decision.
--
To view, visit http://gerrit.cloudera.org:8080/6052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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] [security] security-flags
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [security] security-flags
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/6052/2//COMMIT_MSG
Commit Message:
PS2, Line 20: A follow up commit will hook this flag
: into the RPC system to ensure that authentication is enforced as
: necessary.
keep in mind that clients don't have the ability to configure flags, so we'll need to make it a KuduClientBuilder parameter in those cases. (also to allow a single client to talk to multiple clusters).
(same below)
PS2, Line 42: 1) It's not strictly a server flag, it also applies to
: the kudu CLI tool.
how does it apply to the CLI tool? Given that we don't currently support client-cert authentication for Kudu (BYOPKI), I'm surprised that this is respected by the CLI.
Actually, I think that we should probably remove (or mark experimental/hidden) all of these flags until we officially support and test the BYOPKI option). Their main purpose today is to enable Impala, and if we removed them and provided APIs, Impala can define them in their own server code and pass them into the Messenger at startup (like we do with the IPKI keys/certs)
PS2, Line 43: 2) It's really long, and the length doesn't add
: useful description or specificity. 3) The short form (cert instead
: of certificate) is common in database CLI configs [1], [2], [3].
> I mentioned this last night in #kudu-security on Slack, but I'll repeat it
yea, I agree that 'rpc_key' isn't very descriptive. At the least I'd expect 'rpc_tls_key' or 'rpc_ssl_key', since who knows what other kinds of keys might exist.
Again, though, may be a moot point since we aren't allowing end users to configure these things.
Line 58: --webserver_certificate_file
this and the below two were chosen for compatibility with Impala. Obviously it's not a strict requirement that we be configured identically, but I think it's easier for operators to deal with if we are.
--
To view, visit http://gerrit.cloudera.org:8080/6052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [security] security-flags
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [security] security-flags
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6052/2//COMMIT_MSG
Commit Message:
Line 58: --webserver_certificate_file
> Alright, another option is to leave the webserver options and change the rp
My vote: consistency with Impala is a good thing, as we have quite a few users who will be configuring both.
As for the rpc settings, I have less of an opinion since those will be experimental. I vote you use your judgement and we can always change them later if we change our minds.
--
To view, visit http://gerrit.cloudera.org:8080/6052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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] [security] security-flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6052
to look at the new patch set (#3).
Change subject: [security] security-flags
......................................................................
[security] security-flags
This commit introduces, removes, and renames security flags in an effort
to make the flags more consistent, more understandable, and easier to
use from the command line. Affected flags:
--rpc_authentication
This is a new flag which will apply to Kudu servers and the kudu
command line tool which will allow operators to configure a policy
for authentication of RPC connections. The possible values are
'enabled', 'disabled', and 'required'. Three states are necessary
(as opposed to just 'disabled' and 'required') in order to provide a
graceful upgrade path for clusters from unsecured to secured.
'enabled' is the default. A follow up commit will hook this flag
into the RPC system to ensure that authentication is enforced as
necessary.
--rpc_encryption
This is a new flag which will apply to Kudu servers and the 'kudu'
command line tool which allows operators to configure a policy for
encryption on RPC connections. This is a tristate flag for the same
reasons as outlined in --rpc_authentication. A follow up commit will
hook this flag into the RPC system to ensure that encryption is
enforced as necessary.
--server_require_kerberos
This flag has been removed, and in it's place the --keytab and
--rpc_authentication=required flags are provided. --keytab is used
to enable Kerberos authentication on a server, and
--rpc_authentication=required is used to ensure that all RPCs use
authentication.
--rpc_cert
--rpc_ssl_server_certificate
--rpc_cert is replacing --rpc_ssl_server_certificate. The latter has
a few issues. 1) It's not strictly a server flag, it also applies to
the kudu CLI tool. 2) It's really long, and the length doesn't add
useful description or specificity. 3) The short form (cert instead
of certificate) is common in database CLI configs [1], [2], [3].
--rpc_key
--rpc_ssl_private_key
--rpc_key is replacing --rpc_ssl_private_key. Same reasons as
--rpc_cert.
--rpc_ca_cert
--rpc_ssl_certificate_authority
--rpc_ca_cert is replacing --rpc_ssl_certificate_authority. Same
reasons as --rpc_cert.
--webserver_cert
--webserver_certificate_file
--webserver_cert is replacing --webserver_certificate file to stay
consistent with the analogous RPC flag.
--webserver_key
--webserver_private_key_file
--webserver_key is replacing --webserver_private_key_file to stay
consistent with the analogous RPC flag.
--webserver_key_password_cmd
--webserver_private_key_password_cmd
--webserver_key_password_cmd is replacing
--webserver_private_key_password_cmd to stay consistent with
--webserver_key.
[1] https://rethinkdb.com/docs/security/#using-tls
[2] https://www.cockroachlabs.com/docs/create-security-certificates.html#flags
[3] https://docs.mongodb.com/manual/tutorial/configure-ssl/
I looked at Postgres and MySQL as well, but both of those look in the
data directory for the cert and key files, so they don't have individual
CLI arguments. However, they both commonly refer to certificates as
certs in the documentation.
Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
---
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/sasl_common.cc
M src/kudu/server/webserver_options.cc
8 files changed, 119 insertions(+), 63 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/6052/3
--
To view, visit http://gerrit.cloudera.org:8080/6052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [security] security-flags
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [security] security-flags
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/6052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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] [security] security-flags
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [security] security-flags
......................................................................
Patch Set 5:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/6052/4/src/kudu/rpc/messenger.cc
File src/kudu/rpc/messenger.cc:
Line 62:
> The new tri-state flags should be tagged as stable.
Done
PS4, Line 84: tions with
> --rpc_certificate_file ?
Done
Line 164: auto cleanup = MakeScopedCleanup([&] () {
> Would be nice to do this validation in a per-gflag validator. Better experi
It does fail 'faster', but this still gets run synchronously as part of the server startup, I think. I originally implemented it as a gflags validator, but there is no way (that I can find) to supply an error message with the validator API, so the error message shown to the user is very generic and unhelpful.
PS4, Line 198: uto* tls_context = new_msgr->mutable_tls_context();
:
: if (!FLAGS_rpc_certificate_file.empty() &&
: !FLAGS_rpc_private_key_file.empty() &&
: !FLAGS_rpc_ca_certificate_file.empty()) {
:
> Likewise with this.
same reasoning.
http://gerrit.cloudera.org:8080/#/c/6052/4/src/kudu/rpc/negotiation.cc
File src/kudu/rpc/negotiation.cc:
Line 206: // TODO(PKI): this should be enabling PLAIN if authn < required, and GSSAPI if
> shouldn't this be checking whether the authentication = 'required'? for 'en
Yes I think you are correct. That being said, there are a bunch of places where this needs to be fixed up, so I'm going to add a TODO and punt for now. I'll come back soon and make more comprehensive fixes for this and related issues.
--
To view, visit http://gerrit.cloudera.org:8080/6052
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa53348b8969e83d9f794e1e0553bdec12252d9a
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@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