You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Xiaomeng Zhang (Code Review)" <ge...@cloudera.org> on 2019/10/21 17:14:15 UTC

[kudu-CR] KUDU-2979 Add wrap up function of krb5 parse name, so it can be called from Impala 

Xiaomeng Zhang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14520


Change subject: KUDU-2979 Add wrap up function of krb5_parse_name, so it can be called from Impala
......................................................................

KUDU-2979 Add wrap up function of krb5_parse_name, so it can be called from Impala

From work of https://issues.apache.org/jira/browse/IMPALA-7504, we want to use  krb5_parse_name() to parse the principal instead of creating our own.

As src/kudu/security/init.cc already have g_krb5_ctx initialized, we want to leverage the code in KUDU, and create a wrap up function which can be called from IMPALA

Krb5parseName(const string& principal, string* service_name,
string* hostname, string* realm)

Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
---
M src/kudu/security/init.cc
M src/kudu/security/init.h
2 files changed, 22 insertions(+), 0 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/14520/1
-- 
To view, visit http://gerrit.cloudera.org:8080/14520
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Gerrit-Change-Number: 14520
Gerrit-PatchSet: 1
Gerrit-Owner: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2979: Add wrapper function of krb5 parse name to be used in Impala

Posted by "Xiaomeng Zhang (Code Review)" <ge...@cloudera.org>.
Xiaomeng Zhang has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/14520 )

Change subject: KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala
......................................................................

KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala

From IMPALA-7504, we want to use krb5_parse_name() to parse the principal
instead of using custom code.

When kerberos is initialized in Impala's copy of Kudu code, it stores a
global context which is used when accessing the Krb5 library. To use
this global context, the code that parses the principal name is moved
into Kudu code. This new code is then called from the existing
ParseKerberosPrincipal method.

Test added in mini_kdc-test.TestBasicOperation.

Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
---
M src/kudu/security/init.cc
M src/kudu/security/init.h
M src/kudu/security/test/mini_kdc-test.cc
3 files changed, 40 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/14520/8
-- 
To view, visit http://gerrit.cloudera.org:8080/14520
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Gerrit-Change-Number: 14520
Gerrit-PatchSet: 8
Gerrit-Owner: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Xiaomeng Zhang <xi...@cloudera.com>

[kudu-CR] KUDU-2979 Add wrap up function of krb5 parse name, so it can be called from Impala

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14520 )

Change subject: KUDU-2979 Add wrap up function of krb5_parse_name, so it can be called from Impala
......................................................................


Patch Set 2:

(5 comments)

I know the intent isn't to use this within Kudu, but could you at least add a simple unit test so we don't accidentally break this functionality for Impala in the future?

http://gerrit.cloudera.org:8080/#/c/14520/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14520/2//COMMIT_MSG@7
PS2, Line 7: KUDU-2979 Add wrap up function of krb5_parse_name, so it can be called from Impala
Format as "KUDU-2979: ..."

Also, I think "wrapper" function is more appropriate than "wrap up". So maybe this should be "Add wrapper function for krb5_parse_name to be used in Impala"?


http://gerrit.cloudera.org:8080/#/c/14520/2//COMMIT_MSG@9
PS2, Line 9:   
Nit: got two spaces here


http://gerrit.cloudera.org:8080/#/c/14520/2//COMMIT_MSG@13
PS2, Line 13: wrap up 
wrapper


http://gerrit.cloudera.org:8080/#/c/14520/2//COMMIT_MSG@16
PS2, Line 16: Krb5parseName(const string& principal, string* service_name,
            : string* hostname, string* realm)
I think this is too much detail given the simplicity of the patch. If someone wants to see the function signature you chose, it's right there in init.h.


http://gerrit.cloudera.org:8080/#/c/14520/2/src/kudu/security/init.h
File src/kudu/security/init.h:

http://gerrit.cloudera.org:8080/#/c/14520/2/src/kudu/security/init.h@38
PS2, Line 38: // Convert a string representation of a principal name to a krb5_principal structure.
            : // And get service_name, hostname, realm from it.
The krb5_principal part seems like an implementation detail not worth documenting. Perhaps:

 // Parses the given Kerberos principal into a service name, hostname, and realm.



-- 
To view, visit http://gerrit.cloudera.org:8080/14520
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Gerrit-Change-Number: 14520
Gerrit-PatchSet: 2
Gerrit-Owner: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 22 Oct 2019 03:55:01 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2979: Add wrapper function of krb5 parse name to be used in Impala

Posted by "Xiaomeng Zhang (Code Review)" <ge...@cloudera.org>.
Xiaomeng Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14520 )

Change subject: KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala
......................................................................


Patch Set 7:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/14520/7/src/kudu/security/test/mini_kdc-test.cc
File src/kudu/security/test/mini_kdc-test.cc:

http://gerrit.cloudera.org:8080/#/c/14520/7/src/kudu/security/test/mini_kdc-test.cc@78
PS7, Line 78:   // Test parse krb5 principal.
            :   string principal = "kudu/foo.example.com@KRBTEST.COM";
            :   string service_name;
            :   string hostname;
            :   string realm;
            :   ASSERT_OK(security::Krb5ParseName(principal, &service_name, &hostname, &realm));
            :   ASSERT_EQ("kudu", service_name);
            :   ASSERT_EQ("foo.example.com", hostname);
            :   ASSERT_EQ("KRBTEST.COM", realm);
> nit: I didn't notice it in the prior revision, but it would nice to join th
Done


http://gerrit.cloudera.org:8080/#/c/14520/7/src/kudu/security/test/mini_kdc-test.cc@90
PS7, Line 90: ASSERT_TRUE(security::Krb5ParseName("kudu@KRBTEST.COM", &service_name, &hostname, &realm).IsInvalidArgument());
> This line it too long: it's over 100 characters (LINT checks failed).  You 
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/14520
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Gerrit-Change-Number: 14520
Gerrit-PatchSet: 7
Gerrit-Owner: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Comment-Date: Mon, 28 Oct 2019 17:46:42 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2979: Add wrapper function of krb5 parse name to be used in Impala

Posted by "Xiaomeng Zhang (Code Review)" <ge...@cloudera.org>.
Xiaomeng Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14520 )

Change subject: KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14520/4/src/kudu/security/init.cc
File src/kudu/security/init.cc:

http://gerrit.cloudera.org:8080/#/c/14520/4/src/kudu/security/init.cc@466
PS4, Line 466:   SCOPED_CLEANUP({
             :       krb5_free_principal(g_krb5_ctx, princ);
             :     });
> This should be moved up to just after the krb5_parse_name() call. The ratio
If I clean up princ early, how can I get realm, service name, host name from it?



-- 
To view, visit http://gerrit.cloudera.org:8080/14520
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Gerrit-Change-Number: 14520
Gerrit-PatchSet: 4
Gerrit-Owner: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Comment-Date: Thu, 24 Oct 2019 23:08:22 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2979: Add wrapper function of krb5 parse name to be used in Impala

Posted by "Xiaomeng Zhang (Code Review)" <ge...@cloudera.org>.
Xiaomeng Zhang has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/14520 )

Change subject: KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala
......................................................................

KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala

From IMPALA-7504, We want to use krb5_parse_name() to parse the principal
instead of using custom code.

When kerberos is initialized in Impala's copy of Kudu code, it stores a
global context which is used when accessing the Krb5 library. To use
this global context, the code that parses the principal name is moved
into Kudu code. This new code is then called from the existing
ParseKerberosPrincipal method.

Test added in Impala authentication-test.cc

Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
---
M src/kudu/security/init.cc
M src/kudu/security/init.h
2 files changed, 21 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/14520/3
-- 
To view, visit http://gerrit.cloudera.org:8080/14520
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Gerrit-Change-Number: 14520
Gerrit-PatchSet: 3
Gerrit-Owner: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2979: Add wrapper function of krb5 parse name to be used in Impala

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14520 )

Change subject: KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala
......................................................................


Patch Set 9: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/14520
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Gerrit-Change-Number: 14520
Gerrit-PatchSet: 9
Gerrit-Owner: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Comment-Date: Mon, 28 Oct 2019 20:18:34 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2979: Add wrapper function of krb5 parse name to be used in Impala

Posted by "Xiaomeng Zhang (Code Review)" <ge...@cloudera.org>.
Xiaomeng Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14520 )

Change subject: KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala
......................................................................


Patch Set 6:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/14520/4/src/kudu/security/init.cc
File src/kudu/security/init.cc:

http://gerrit.cloudera.org:8080/#/c/14520/4/src/kudu/security/init.cc@466
PS4, Line 466:   }
             :   realm->assign(princ->realm.data, princ->realm.length);
             :   servi
> What the SCOPED_CLEANUP block is doing is saying "when I go out of scope, r
Done


http://gerrit.cloudera.org:8080/#/c/14520/5/src/kudu/security/init.cc
File src/kudu/security/init.cc:

http://gerrit.cloudera.org:8080/#/c/14520/5/src/kudu/security/init.cc@464
PS5, Line 464: "bad principal format,
> Does it make sense to add information on what was the input string principa
Done


http://gerrit.cloudera.org:8080/#/c/14520/4/src/kudu/security/test/mini_kdc-test.cc
File src/kudu/security/test/mini_kdc-test.cc:

http://gerrit.cloudera.org:8080/#/c/14520/4/src/kudu/security/test/mini_kdc-test.cc@79
PS4, Line 79:   string principal = "kudu/foo.example.com@KRBTEST.COM";
            :   string service_name;
            :   string hostname;
            :   string realm;
> In Kudu, only class members are suffixed with an underscore. Regular locals
Done


http://gerrit.cloudera.org:8080/#/c/14520/5/src/kudu/security/test/mini_kdc-test.cc
File src/kudu/security/test/mini_kdc-test.cc:

http://gerrit.cloudera.org:8080/#/c/14520/5/src/kudu/security/test/mini_kdc-test.cc@78
PS5, Line 78:   // Test parse krb5 principal.
> This is a good test for the 'happy' or 'positive' case.
So the requirements is that principal must have service_name, hostname and realm. I find that mini_kdc has default realm. So what we can test is empty string, and missing service_name or hostname.



-- 
To view, visit http://gerrit.cloudera.org:8080/14520
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Gerrit-Change-Number: 14520
Gerrit-PatchSet: 6
Gerrit-Owner: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Comment-Date: Fri, 25 Oct 2019 22:15:45 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2979: Add wrapper function of krb5 parse name to be used in Impala

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14520 )

Change subject: KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala
......................................................................


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/14520/5/src/kudu/security/init.cc
File src/kudu/security/init.cc:

http://gerrit.cloudera.org:8080/#/c/14520/5/src/kudu/security/init.cc@464
PS5, Line 464: "bad principal format"
Does it make sense to add information on what was the input string principal and what's exactly wrong with the result?


http://gerrit.cloudera.org:8080/#/c/14520/5/src/kudu/security/test/mini_kdc-test.cc
File src/kudu/security/test/mini_kdc-test.cc:

http://gerrit.cloudera.org:8080/#/c/14520/5/src/kudu/security/test/mini_kdc-test.cc@78
PS5, Line 78:   // Test parse krb5 principal.
This is a good test for the 'happy' or 'positive' case.

I think it would be nice to add a few other cases to be explicit on our expectations what's returned in such cases:
  * shortened and service principals, like 'bar', 'bar@KRBTEST.COM', `kudu/foo.example.com`
  * malformed/unparsable principals
  * empty principal (i.e. empty string)



-- 
To view, visit http://gerrit.cloudera.org:8080/14520
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Gerrit-Change-Number: 14520
Gerrit-PatchSet: 5
Gerrit-Owner: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Comment-Date: Fri, 25 Oct 2019 19:31:41 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2979: Add wrapper function of krb5 parse name to be used in Impala

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14520 )

Change subject: KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14520/4/src/kudu/security/init.cc
File src/kudu/security/init.cc:

http://gerrit.cloudera.org:8080/#/c/14520/4/src/kudu/security/init.cc@466
PS4, Line 466:   SCOPED_CLEANUP({
             :       krb5_free_principal(g_krb5_ctx, princ);
             :     });
> If I clean up princ early, how can I get realm, service name, host name fro
What the SCOPED_CLEANUP block is doing is saying "when I go out of scope, run this lambda". It's like if you had allocated memory use the new operator and then stored the result in a unique_ptr: the deallocation doesn't happen at the site of the unique_ptr declaration, it happens when the unique_ptr goes out of scope and is destroyed.



-- 
To view, visit http://gerrit.cloudera.org:8080/14520
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Gerrit-Change-Number: 14520
Gerrit-PatchSet: 4
Gerrit-Owner: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Comment-Date: Thu, 24 Oct 2019 23:15:29 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2979: Add wrapper function of krb5 parse name to be used in Impala

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14520 )

Change subject: KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala
......................................................................


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/14520/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14520/2//COMMIT_MSG@16
PS2, Line 16: ParseKerberosPrincipal method.
            : 
> The issue is that without Kudu tests we won't know if we've broken this fun
+1


http://gerrit.cloudera.org:8080/#/c/14520/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14520/3//COMMIT_MSG@9
PS3, Line 9: We
nit: we


http://gerrit.cloudera.org:8080/#/c/14520/3/src/kudu/security/init.cc
File src/kudu/security/init.cc:

http://gerrit.cloudera.org:8080/#/c/14520/3/src/kudu/security/init.cc@466
PS3, Line 466:   krb5_free_principal(g_krb5_ctx, princ);
I think it's cleaner to add a scoped clean-up just after calling krb5_parse_name():

  SCOPED_CLEANUP({
      krb5_free_principal(g_krb5_ctx, princ);
    });



-- 
To view, visit http://gerrit.cloudera.org:8080/14520
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Gerrit-Change-Number: 14520
Gerrit-PatchSet: 3
Gerrit-Owner: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Oct 2019 17:24:25 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2979: Add wrapper function of krb5 parse name to be used in Impala

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14520 )

Change subject: KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14520/7/src/kudu/security/test/mini_kdc-test.cc
File src/kudu/security/test/mini_kdc-test.cc:

http://gerrit.cloudera.org:8080/#/c/14520/7/src/kudu/security/test/mini_kdc-test.cc@90
PS7, Line 90: ASSERT_TRUE(security::Krb5ParseName("kudu@KRBTEST.COM", &service_name, &hostname, &realm).IsInvalidArgument());
This line it too long: it's over 100 characters (LINT checks failed).  You need to wrap it like:

ASSERT_TRUE(security::Krb5ParseName("kudu@KRBTEST.COM", &service_name,           
    &hostname, &realm).IsInvalidArgument());



-- 
To view, visit http://gerrit.cloudera.org:8080/14520
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Gerrit-Change-Number: 14520
Gerrit-PatchSet: 7
Gerrit-Owner: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Comment-Date: Sat, 26 Oct 2019 02:05:12 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2979: Add wrapper function of krb5 parse name to be used in Impala

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14520 )

Change subject: KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala
......................................................................


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/14520/6/src/kudu/security/init.cc
File src/kudu/security/init.cc:

http://gerrit.cloudera.org:8080/#/c/14520/6/src/kudu/security/init.cc@464
PS6, Line 464: "bad principal format");
             :   }
Could you also add the original principal string into the error message?  That could help in troubleshooting if this error is ever triggered.

That would be something like:

  return Status::InvalidArgument(Substitute("$0: principal should include service name, hostname and realm", principal));


http://gerrit.cloudera.org:8080/#/c/14520/5/src/kudu/security/test/mini_kdc-test.cc
File src/kudu/security/test/mini_kdc-test.cc:

http://gerrit.cloudera.org:8080/#/c/14520/5/src/kudu/security/test/mini_kdc-test.cc@78
PS5, Line 78:   // Test parse krb5 principal.
> So the requirements is that principal must have service_name, hostname and 
Thanks!

Then maybe add the case of `kudu/foo.example.com` as a 'happy' case as well, please.



-- 
To view, visit http://gerrit.cloudera.org:8080/14520
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Gerrit-Change-Number: 14520
Gerrit-PatchSet: 5
Gerrit-Owner: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Comment-Date: Fri, 25 Oct 2019 23:12:20 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2979: Add wrapper function of krb5 parse name to be used in Impala

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14520 )

Change subject: KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14520/3/src/kudu/security/init.cc
File src/kudu/security/init.cc:

http://gerrit.cloudera.org:8080/#/c/14520/3/src/kudu/security/init.cc@460
PS3, Line 460: realm.data
> I checked that the char array ends with 0'\000'.
A loop is not needed.  There is a constructor of signature like basic_string(const char*, size_t len).

For details, see https://en.cppreference.com/w/cpp/string/basic_string/basic_string



-- 
To view, visit http://gerrit.cloudera.org:8080/14520
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Gerrit-Change-Number: 14520
Gerrit-PatchSet: 4
Gerrit-Owner: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Comment-Date: Thu, 24 Oct 2019 22:48:39 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2979: Add wrapper function of krb5 parse name to be used in Impala

Posted by "Xiaomeng Zhang (Code Review)" <ge...@cloudera.org>.
Xiaomeng Zhang has uploaded a new patch set (#9). ( http://gerrit.cloudera.org:8080/14520 )

Change subject: KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala
......................................................................

KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala

From IMPALA-7504, we want to use krb5_parse_name() to parse the principal
instead of using custom code.

When kerberos is initialized in Impala's copy of Kudu code, it stores a
global context which is used when accessing the Krb5 library. To use
this global context, the code that parses the principal name is moved
into Kudu code. This new code is then called from the existing
ParseKerberosPrincipal method.

Test added in mini_kdc-test.TestBasicOperation.

Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
---
M src/kudu/security/init.cc
M src/kudu/security/init.h
M src/kudu/security/test/mini_kdc-test.cc
3 files changed, 40 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/14520/9
-- 
To view, visit http://gerrit.cloudera.org:8080/14520
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Gerrit-Change-Number: 14520
Gerrit-PatchSet: 9
Gerrit-Owner: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Xiaomeng Zhang <xi...@cloudera.com>

[kudu-CR] KUDU-2979: Add wrapper function of krb5 parse name to be used in Impala

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14520 )

Change subject: KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/14520/3/src/kudu/security/init.cc
File src/kudu/security/init.cc:

http://gerrit.cloudera.org:8080/#/c/14520/3/src/kudu/security/init.cc@460
PS3, Line 460: realm.data
Here and elsewhere below: is this guaranteed that 'data' field of the krb5_data structure is null-terminated?

Why not to use krb5_data.length field when copying from a krb5_data.data C-string?  Would it be safer?


http://gerrit.cloudera.org:8080/#/c/14520/3/src/kudu/security/init.cc@461
PS3, Line 461: krb5_data*
nit: add 'const'
  const krb5_data*



-- 
To view, visit http://gerrit.cloudera.org:8080/14520
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Gerrit-Change-Number: 14520
Gerrit-PatchSet: 3
Gerrit-Owner: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Oct 2019 17:44:14 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2979: Add wrapper function of krb5 parse name to be used in Impala

Posted by "Xiaomeng Zhang (Code Review)" <ge...@cloudera.org>.
Xiaomeng Zhang has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/14520 )

Change subject: KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala
......................................................................

KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala

From IMPALA-7504, we want to use krb5_parse_name() to parse the principal
instead of using custom code.

When kerberos is initialized in Impala's copy of Kudu code, it stores a
global context which is used when accessing the Krb5 library. To use
this global context, the code that parses the principal name is moved
into Kudu code. This new code is then called from the existing
ParseKerberosPrincipal method.

Test added in mini_kdc-test.TestBasicOperation.

Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
---
M src/kudu/security/init.cc
M src/kudu/security/init.h
M src/kudu/security/test/mini_kdc-test.cc
3 files changed, 33 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/14520/4
-- 
To view, visit http://gerrit.cloudera.org:8080/14520
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Gerrit-Change-Number: 14520
Gerrit-PatchSet: 4
Gerrit-Owner: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Xiaomeng Zhang <xi...@cloudera.com>

[kudu-CR] KUDU-2979: Add wrapper function of krb5 parse name to be used in Impala

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14520 )

Change subject: KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala
......................................................................


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/14520/4/src/kudu/security/init.cc
File src/kudu/security/init.cc:

http://gerrit.cloudera.org:8080/#/c/14520/4/src/kudu/security/init.cc@466
PS4, Line 466:   SCOPED_CLEANUP({
             :       krb5_free_principal(g_krb5_ctx, princ);
             :     });
This should be moved up to just after the krb5_parse_name() call. The rationale is: we want to make sure we free princ if _anything_ were to fail after it's allocated. To ensure that, the cleanup block must be declared as soon as princ is allocated.

This is more philosophical than practical: the few lines of code between krb5_parse_name and the end of the function aren't going to early return or fail. But in projects that make use of exceptions and operator overloading, it's possible for seemingly innocuous operations like data++ to throw an exception, at which point you want to make sure the cleanup block is declared before that happens (so that cleanup runs as the function stack is unwound).


http://gerrit.cloudera.org:8080/#/c/14520/4/src/kudu/security/test/mini_kdc-test.cc
File src/kudu/security/test/mini_kdc-test.cc:

http://gerrit.cloudera.org:8080/#/c/14520/4/src/kudu/security/test/mini_kdc-test.cc@79
PS4, Line 79:   string principal_ = "kudu/foo.example.com@KRBTEST.COM";
            :   string service_name_;
            :   string host_name_;
            :   string realm_;
In Kudu, only class members are suffixed with an underscore. Regular locals should not do that.



-- 
To view, visit http://gerrit.cloudera.org:8080/14520
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Gerrit-Change-Number: 14520
Gerrit-PatchSet: 4
Gerrit-Owner: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Comment-Date: Thu, 24 Oct 2019 22:25:17 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2979 Add wrap up function of krb5 parse name, so it can be called from Impala

Posted by "Xiaomeng Zhang (Code Review)" <ge...@cloudera.org>.
Xiaomeng Zhang has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/14520 )

Change subject: KUDU-2979 Add wrap up function of krb5_parse_name, so it can be called from Impala
......................................................................

KUDU-2979 Add wrap up function of krb5_parse_name, so it can be called from Impala

From work IMPALA-7504, we want to use  krb5_parse_name() to parse the
principal instead of creating our own.

As src/kudu/security/init.cc already have g_krb5_ctx initialized,
we want to leverage the code in KUDU, and create a wrap up function
which can be called from IMPALA

Krb5parseName(const string& principal, string* service_name,
string* hostname, string* realm)

Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
---
M src/kudu/security/init.cc
M src/kudu/security/init.h
2 files changed, 22 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/14520/2
-- 
To view, visit http://gerrit.cloudera.org:8080/14520
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Gerrit-Change-Number: 14520
Gerrit-PatchSet: 2
Gerrit-Owner: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2979: Add wrapper function of krb5 parse name to be used in Impala

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14520 )

Change subject: KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala
......................................................................

KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala

From IMPALA-7504, we want to use krb5_parse_name() to parse the principal
instead of using custom code.

When kerberos is initialized in Impala's copy of Kudu code, it stores a
global context which is used when accessing the Krb5 library. To use
this global context, the code that parses the principal name is moved
into Kudu code. This new code is then called from the existing
ParseKerberosPrincipal method.

Test added in mini_kdc-test.TestBasicOperation.

Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Reviewed-on: http://gerrit.cloudera.org:8080/14520
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/security/init.cc
M src/kudu/security/init.h
M src/kudu/security/test/mini_kdc-test.cc
3 files changed, 40 insertions(+), 0 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Alexey Serbin: Looks good to me, approved

-- 
To view, visit http://gerrit.cloudera.org:8080/14520
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Gerrit-Change-Number: 14520
Gerrit-PatchSet: 10
Gerrit-Owner: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Xiaomeng Zhang <xi...@cloudera.com>

[kudu-CR] KUDU-2979: Add wrapper function of krb5 parse name to be used in Impala

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14520 )

Change subject: KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala
......................................................................


Patch Set 7: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14520/7/src/kudu/security/test/mini_kdc-test.cc
File src/kudu/security/test/mini_kdc-test.cc:

http://gerrit.cloudera.org:8080/#/c/14520/7/src/kudu/security/test/mini_kdc-test.cc@78
PS7, Line 78:   // Test parse krb5 principal.
            :   string principal = "kudu/foo.example.com@KRBTEST.COM";
            :   string service_name;
            :   string hostname;
            :   string realm;
            :   ASSERT_OK(security::Krb5ParseName(principal, &service_name, &hostname, &realm));
            :   ASSERT_EQ("kudu", service_name);
            :   ASSERT_EQ("foo.example.com", hostname);
            :   ASSERT_EQ("KRBTEST.COM", realm);
nit: I didn't notice it in the prior revision, but it would nice to join this and the additional 'happy' case at lines 92-93 into the following:

for (const auto& principal : { "kudu/foo.example.com@KRBTEST.COM", "kudu/foo.example" }) {
  string service_name;
  string hostname;
  string realm;
  ASSERT_OK(security::Krb5ParseName(principal, &service_name, &hostname, &realm));
  ASSERT_EQ("kudu", service_name);
  ASSERT_EQ("foo.example.com", hostname);
  ASSERT_EQ("KRBTEST.COM", realm);
}



-- 
To view, visit http://gerrit.cloudera.org:8080/14520
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Gerrit-Change-Number: 14520
Gerrit-PatchSet: 7
Gerrit-Owner: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Comment-Date: Sat, 26 Oct 2019 02:01:22 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2979: Add wrapper function of krb5 parse name to be used in Impala

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14520 )

Change subject: KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14520/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14520/2//COMMIT_MSG@16
PS2, Line 16: ParseKerberosPrincipal method.
            : 
> I added test in impala https://gerrit.cloudera.org/#/c/14433/. For kudu, is
The issue is that without Kudu tests we won't know if we've broken this function until Impala next merges changes from krpc. It'll be frustrating to deal with breakages at that time.

How about making changes to mini_kdc-test? TestBasicOperation seems like a great place to call the new function, as it already does a bunch of stuff that sets up a krb5 context.



-- 
To view, visit http://gerrit.cloudera.org:8080/14520
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Gerrit-Change-Number: 14520
Gerrit-PatchSet: 3
Gerrit-Owner: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Oct 2019 03:42:40 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2979: Add wrapper function of krb5 parse name to be used in Impala

Posted by "Xiaomeng Zhang (Code Review)" <ge...@cloudera.org>.
Xiaomeng Zhang has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/14520 )

Change subject: KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala
......................................................................

KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala

From IMPALA-7504, we want to use krb5_parse_name() to parse the principal
instead of using custom code.

When kerberos is initialized in Impala's copy of Kudu code, it stores a
global context which is used when accessing the Krb5 library. To use
this global context, the code that parses the principal name is moved
into Kudu code. This new code is then called from the existing
ParseKerberosPrincipal method.

Test added in mini_kdc-test.TestBasicOperation.

Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
---
M src/kudu/security/init.cc
M src/kudu/security/init.h
M src/kudu/security/test/mini_kdc-test.cc
3 files changed, 41 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/14520/7
-- 
To view, visit http://gerrit.cloudera.org:8080/14520
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Gerrit-Change-Number: 14520
Gerrit-PatchSet: 7
Gerrit-Owner: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Xiaomeng Zhang <xi...@cloudera.com>

[kudu-CR] KUDU-2979: Add wrapper function of krb5 parse name to be used in Impala

Posted by "Xiaomeng Zhang (Code Review)" <ge...@cloudera.org>.
Xiaomeng Zhang has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/14520 )

Change subject: KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala
......................................................................

KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala

From IMPALA-7504, we want to use krb5_parse_name() to parse the principal
instead of using custom code.

When kerberos is initialized in Impala's copy of Kudu code, it stores a
global context which is used when accessing the Krb5 library. To use
this global context, the code that parses the principal name is moved
into Kudu code. This new code is then called from the existing
ParseKerberosPrincipal method.

Test added in mini_kdc-test.TestBasicOperation.

Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
---
M src/kudu/security/init.cc
M src/kudu/security/init.h
M src/kudu/security/test/mini_kdc-test.cc
3 files changed, 33 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/14520/5
-- 
To view, visit http://gerrit.cloudera.org:8080/14520
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Gerrit-Change-Number: 14520
Gerrit-PatchSet: 5
Gerrit-Owner: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Xiaomeng Zhang <xi...@cloudera.com>

[kudu-CR] KUDU-2979: Add wrapper function of krb5 parse name to be used in Impala

Posted by "Xiaomeng Zhang (Code Review)" <ge...@cloudera.org>.
Xiaomeng Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14520 )

Change subject: KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala
......................................................................


Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/14520/3/src/kudu/security/init.cc
File src/kudu/security/init.cc:

http://gerrit.cloudera.org:8080/#/c/14520/3/src/kudu/security/init.cc@460
PS3, Line 460: realm.data
> Here and elsewhere below: is this guaranteed that 'data' field of the krb5_
I checked that the char array ends with 0'\000'.
For using length, do you mean using a for loop? Is there a one line function to convert char array to string?


http://gerrit.cloudera.org:8080/#/c/14520/3/src/kudu/security/init.cc@461
PS3, Line 461: const krb5
> nit: add 'const'
Done


http://gerrit.cloudera.org:8080/#/c/14520/3/src/kudu/security/init.cc@466
PS3, Line 466:   SCOPED_CLEANUP({
> I think it's cleaner to add a scoped clean-up just after calling krb5_parse
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/14520
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Gerrit-Change-Number: 14520
Gerrit-PatchSet: 4
Gerrit-Owner: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Comment-Date: Thu, 24 Oct 2019 22:24:39 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2979: Add wrapper function of krb5 parse name to be used in Impala

Posted by "Xiaomeng Zhang (Code Review)" <ge...@cloudera.org>.
Xiaomeng Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14520 )

Change subject: KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala
......................................................................


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/14520/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14520/2//COMMIT_MSG@7
PS2, Line 7: KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala
> Format as "KUDU-2979: ..."
Done


http://gerrit.cloudera.org:8080/#/c/14520/2//COMMIT_MSG@9
PS2, Line 9: _p
> Nit: got two spaces here
Done


http://gerrit.cloudera.org:8080/#/c/14520/2//COMMIT_MSG@16
PS2, Line 16: ParseKerberosPrincipal method.
            : 
> I think this is too much detail given the simplicity of the patch. If someo
I added test in impala https://gerrit.cloudera.org/#/c/14433/. For kudu, is there any test class I can use to add test? What I find in kudu is using kdc.Kinit().


http://gerrit.cloudera.org:8080/#/c/14520/2/src/kudu/security/init.h
File src/kudu/security/init.h:

http://gerrit.cloudera.org:8080/#/c/14520/2/src/kudu/security/init.h@38
PS2, Line 38: // Parses the given Kerberos principal into service name, hostname, and realm.
            : Status Krb5ParseName(const std::string& principal
> The krb5_principal part seems like an implementation detail not worth docum
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/14520
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Gerrit-Change-Number: 14520
Gerrit-PatchSet: 3
Gerrit-Owner: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Oct 2019 01:17:58 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2979: Add wrapper function of krb5 parse name to be used in Impala

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14520 )

Change subject: KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala
......................................................................


Patch Set 7: Code-Review+1

You need to fix the LINT issue at least to make the build passing.


-- 
To view, visit http://gerrit.cloudera.org:8080/14520
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Gerrit-Change-Number: 14520
Gerrit-PatchSet: 7
Gerrit-Owner: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Comment-Date: Sat, 26 Oct 2019 02:14:49 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2979: Add wrapper function of krb5 parse name to be used in Impala

Posted by "Xiaomeng Zhang (Code Review)" <ge...@cloudera.org>.
Xiaomeng Zhang has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/14520 )

Change subject: KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala
......................................................................

KUDU-2979: Add wrapper function of krb5_parse_name to be used in Impala

From IMPALA-7504, we want to use krb5_parse_name() to parse the principal
instead of using custom code.

When kerberos is initialized in Impala's copy of Kudu code, it stores a
global context which is used when accessing the Krb5 library. To use
this global context, the code that parses the principal name is moved
into Kudu code. This new code is then called from the existing
ParseKerberosPrincipal method.

Test added in mini_kdc-test.TestBasicOperation.

Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
---
M src/kudu/security/init.cc
M src/kudu/security/init.h
M src/kudu/security/test/mini_kdc-test.cc
3 files changed, 38 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/14520/6
-- 
To view, visit http://gerrit.cloudera.org:8080/14520
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifddafa7aae25d66ed7d9fa0306f17501a191cdac
Gerrit-Change-Number: 14520
Gerrit-PatchSet: 6
Gerrit-Owner: Xiaomeng Zhang <xi...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Xiaomeng Zhang <xi...@cloudera.com>