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/01/24 16:30:48 UTC
[kudu-CR] [security] separated OpenSSL wrappers from CA
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5779
Change subject: [security] separated OpenSSL wrappers from CA
......................................................................
[security] separated OpenSSL wrappers from CA
Moved common wrappers around OpenSSL objects into a separate header.
Originally, they were in security/ca subdirectory along with CA-related
code. The motivation for this change is to allow other components
to use those wrappers without dependency on other CA-related code.
Change-Id: I37729a865739c5702e92238b9d2375f5bf71543d
---
M src/kudu/security/CMakeLists.txt
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
A src/kudu/security/openssl_traits.h
A src/kudu/security/openssl_wrappers.cc
A src/kudu/security/openssl_wrappers.h
6 files changed, 449 insertions(+), 309 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/5779/1
--
To view, visit http://gerrit.cloudera.org:8080/5779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I37729a865739c5702e92238b9d2375f5bf71543d
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [security] separated OpenSSL wrappers from CA
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [security] separated OpenSSL wrappers from CA
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5779/3/src/kudu/security/openssl_traits.h
File src/kudu/security/openssl_traits.h:
Line 52: template<class SSL_TYPE>
> Right, this is top-level definition for the traits (non-specs). I was thin
I don't feel strongly about whether they are put into openssl_util or not, but I think the definitions should be kept with the declarations, if possible. Otherwise it's not clear where the definitions are just by reading this header.
--
To view, visit http://gerrit.cloudera.org:8080/5779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I37729a865739c5702e92238b9d2375f5bf71543d
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [security] separated OpenSSL wrappers from CA
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [security] separated OpenSSL wrappers from CA
......................................................................
Patch Set 2:
> My concern was that there is some code in TLS socket layer which would need openssl_util.h, but not those wrappers.
I think in fact I will use these wrappers in the TLS socket code when it lands, so I think openssl_util is a good place for it.
--
To view, visit http://gerrit.cloudera.org:8080/5779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I37729a865739c5702e92238b9d2375f5bf71543d
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [security] separated OpenSSL wrappers from CA
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has uploaded a new patch set (#2).
Change subject: [security] separated OpenSSL wrappers from CA
......................................................................
[security] separated OpenSSL wrappers from CA
Moved common wrappers around OpenSSL objects into a separate header.
Originally, they were in security/ca subdirectory along with CA-related
code. The motivation for this change is to allow other components
to use those wrappers without dependency on other CA-related code.
Change-Id: I37729a865739c5702e92238b9d2375f5bf71543d
---
M src/kudu/security/CMakeLists.txt
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
A src/kudu/security/openssl_traits.h
A src/kudu/security/openssl_wrappers.cc
A src/kudu/security/openssl_wrappers.h
6 files changed, 446 insertions(+), 309 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/5779/2
--
To view, visit http://gerrit.cloudera.org:8080/5779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I37729a865739c5702e92238b9d2375f5bf71543d
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [security] separated OpenSSL wrappers from CA
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/5779
to look at the new patch set (#3).
Change subject: [security] separated OpenSSL wrappers from CA
......................................................................
[security] separated OpenSSL wrappers from CA
Moved common wrappers around OpenSSL objects from cert_management.h
into a openssl_util.h header. The motivation for this change is to
allow other components to use those generic wrappers without dependency
on CA-specific code.
Change-Id: I37729a865739c5702e92238b9d2375f5bf71543d
---
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
A src/kudu/security/openssl_traits.h
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
5 files changed, 378 insertions(+), 309 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/5779/3
--
To view, visit http://gerrit.cloudera.org:8080/5779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I37729a865739c5702e92238b9d2375f5bf71543d
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [security] separated OpenSSL wrappers from CA
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [security] separated OpenSSL wrappers from CA
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I37729a865739c5702e92238b9d2375f5bf71543d
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [security] separated OpenSSL wrappers from CA
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [security] separated OpenSSL wrappers from CA
......................................................................
Patch Set 2:
Did you consider putting all of this in openssl_util.h? Seems like an appropriate place for it that already exists.
--
To view, visit http://gerrit.cloudera.org:8080/5779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I37729a865739c5702e92238b9d2375f5bf71543d
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [security] separated OpenSSL wrappers from CA
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [security] separated OpenSSL wrappers from CA
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5779/1/src/kudu/security/openssl_traits.h
File src/kudu/security/openssl_traits.h:
Line 55: } // namespace kudu
> warning: namespace 'security' ends with a comment that refers to a wrong na
Done
Line 56: } // namespace security
> warning: namespace 'kudu' ends with a comment that refers to a wrong namesp
Done
http://gerrit.cloudera.org:8080/#/c/5779/1/src/kudu/security/openssl_wrappers.cc
File src/kudu/security/openssl_wrappers.cc:
Line 40: using std::shared_ptr;
> warning: using decl 'shared_ptr' is unused [misc-unused-using-decls]
Done
--
To view, visit http://gerrit.cloudera.org:8080/5779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I37729a865739c5702e92238b9d2375f5bf71543d
Gerrit-PatchSet: 1
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [security] separated OpenSSL wrappers from CA
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/5779
to look at the new patch set (#4).
Change subject: [security] separated OpenSSL wrappers from CA
......................................................................
[security] separated OpenSSL wrappers from CA
Moved common wrappers around OpenSSL objects from cert_management.h
into a openssl_util.h header. The motivation for this change is to
allow other components to use those generic wrappers without dependency
on CA-specific code.
Change-Id: I37729a865739c5702e92238b9d2375f5bf71543d
---
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
4 files changed, 347 insertions(+), 309 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/5779/4
--
To view, visit http://gerrit.cloudera.org:8080/5779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I37729a865739c5702e92238b9d2375f5bf71543d
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [security] separated OpenSSL wrappers from CA
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [security] separated OpenSSL wrappers from CA
......................................................................
Patch Set 2:
> Did you consider putting all of this in openssl_util.h? Seems like
> an appropriate place for it that already exists.
Yep, but I was not sure that's the best place for it. However, if you are OK with that, I'll move it there.
My concern was that there is some code in TLS socket layer which would need openssl_util.h, but not those wrappers.
--
To view, visit http://gerrit.cloudera.org:8080/5779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I37729a865739c5702e92238b9d2375f5bf71543d
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [security] separated OpenSSL wrappers from CA
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged.
Change subject: [security] separated OpenSSL wrappers from CA
......................................................................
[security] separated OpenSSL wrappers from CA
Moved common wrappers around OpenSSL objects from cert_management.h
into a openssl_util.h header. The motivation for this change is to
allow other components to use those generic wrappers without dependency
on CA-specific code.
Change-Id: I37729a865739c5702e92238b9d2375f5bf71543d
Reviewed-on: http://gerrit.cloudera.org:8080/5779
Reviewed-by: Dan Burkert <da...@apache.org>
Tested-by: Kudu Jenkins
---
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
4 files changed, 347 insertions(+), 309 deletions(-)
Approvals:
Dan Burkert: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I37729a865739c5702e92238b9d2375f5bf71543d
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [security] separated OpenSSL wrappers from CA
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [security] separated OpenSSL wrappers from CA
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5779/3/src/kudu/security/openssl_traits.h
File src/kudu/security/openssl_traits.h:
Line 52: template<class SSL_TYPE>
Could this file be consolidated to openssl_util as well? The definitions for the type traits are, right?
--
To view, visit http://gerrit.cloudera.org:8080/5779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I37729a865739c5702e92238b9d2375f5bf71543d
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [security] separated OpenSSL wrappers from CA
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [security] separated OpenSSL wrappers from CA
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5779/3/src/kudu/security/openssl_traits.h
File src/kudu/security/openssl_traits.h:
Line 52: template<class SSL_TYPE>
> I don't feel strongly about whether they are put into openssl_util or not,
This is a special case, I would say.
I put those in here because they are used in openssl_util.cc and ca/cert_management.cc, and there are internal. Ideally, those should be kept in those *.cc files, but having them in both files would be code duplication.
May be, I should name this as openssl_util-inl.h ?
--
To view, visit http://gerrit.cloudera.org:8080/5779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I37729a865739c5702e92238b9d2375f5bf71543d
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [security] separated OpenSSL wrappers from CA
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [security] separated OpenSSL wrappers from CA
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5779/3/src/kudu/security/openssl_traits.h
File src/kudu/security/openssl_traits.h:
Line 52: template<class SSL_TYPE>
> Could this file be consolidated to openssl_util as well? The definitions f
Right, this is top-level definition for the traits (non-specs). I was thinking to have this header just for internal consumers in libsecurity -- the traits and the macros are not needed outside of the library code.
Or you are expecting them to be useful elsewhere as well?
--
To view, visit http://gerrit.cloudera.org:8080/5779
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I37729a865739c5702e92238b9d2375f5bf71543d
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes