[Impala-ASF-CR] IMPALA-7504 ParseKerberosPrincipal() should use krb5 parse name() instead

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14433 )

Change subject: IMPALA-7504 ParseKerberosPrincipal() should use 
krb5_parse_name() instead
..


Patch Set 4:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/4847/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e64ebdc10f102dbdc5b87f6fe3f2a0310b1be24
Gerrit-Change-Number: 14433
Gerrit-PatchSet: 4
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Xiaomeng Zhang 
Gerrit-Comment-Date: Mon, 21 Oct 2019 21:01:24 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7504 ParseKerberosPrincipal() should use krb5 parse name() instead

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14433 )

Change subject: IMPALA-7504 ParseKerberosPrincipal() should use 
krb5_parse_name() instead
..


Patch Set 3:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/4846/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e64ebdc10f102dbdc5b87f6fe3f2a0310b1be24
Gerrit-Change-Number: 14433
Gerrit-PatchSet: 3
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Xiaomeng Zhang 
Gerrit-Comment-Date: Mon, 21 Oct 2019 20:55:27 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7504 ParseKerberosPrincipal() should use krb5 parse name() instead

[Xiaomeng Zhang (Code Review)]

Xiaomeng Zhang has uploaded a new patch set (#4). ( 
http://gerrit.cloudera.org:8080/14433 )

Change subject: IMPALA-7504 ParseKerberosPrincipal() should use 
krb5_parse_name() instead
..

IMPALA-7504 ParseKerberosPrincipal() should use krb5_parse_name() instead

Change-Id: I0e64ebdc10f102dbdc5b87f6fe3f2a0310b1be24
---
M be/src/kudu/security/init.cc
M be/src/kudu/security/init.h
M be/src/rpc/authentication-test.cc
M be/src/util/auth-util.cc
4 files changed, 36 insertions(+), 15 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/33/14433/4
--
To view, visit http://gerrit.cloudera.org:8080/14433
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0e64ebdc10f102dbdc5b87f6fe3f2a0310b1be24
Gerrit-Change-Number: 14433
Gerrit-PatchSet: 4
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Xiaomeng Zhang 

[Impala-ASF-CR] IMPALA-7504 ParseKerberosPrincipal() should use krb5 parse name() instead

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14433 )

Change subject: IMPALA-7504 ParseKerberosPrincipal() should use 
krb5_parse_name() instead
..


Patch Set 4:

(1 comment)

Commit message still needs improvement

http://gerrit.cloudera.org:8080/#/c/14433/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14433/4//COMMIT_MSG@7
PS4, Line 7: IMPALA-7504 ParseKerberosPrincipal() should use krb5_parse_name() 
instead
See comments on commit msg in patch set 2.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e64ebdc10f102dbdc5b87f6fe3f2a0310b1be24
Gerrit-Change-Number: 14433
Gerrit-PatchSet: 4
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Xiaomeng Zhang 
Gerrit-Comment-Date: Mon, 21 Oct 2019 20:24:20 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7504 ParseKerberosPrincipal() should use krb5 parse name() instead

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14433 )

Change subject: IMPALA-7504 ParseKerberosPrincipal() should use 
krb5_parse_name() instead
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/14433/4/be/src/util/auth-util.cc
File be/src/util/auth-util.cc:

http://gerrit.cloudera.org:8080/#/c/14433/4/be/src/util/auth-util.cc@91
PS4, Line 91:   KUDU_RETURN_IF_ERROR(kudu::security::Krb5ParseName(principal, 
service_name, hostname, realm),
line too long (95 > 90)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e64ebdc10f102dbdc5b87f6fe3f2a0310b1be24
Gerrit-Change-Number: 14433
Gerrit-PatchSet: 4
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Xiaomeng Zhang 
Gerrit-Comment-Date: Mon, 21 Oct 2019 20:15:51 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7504 ParseKerberosPrincipal() should use krb5 parse name() instead

[Xiaomeng Zhang (Code Review)]

Xiaomeng Zhang has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14433 )

Change subject: IMPALA-7504 ParseKerberosPrincipal() should use 
krb5_parse_name() instead
..


Patch Set 3:

(8 comments)

Patch set 3 has jenkins test failure  "Bad SAM flags in obtain_sam_padata" on  
CHECK_EQ(krb5_get_default_realm(krb5_ctx, _realm), 0);

http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc
File be/src/util/auth-util.cc:

http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@90
PS2, Line 90:   krb5_context krb5_ctx;
> Maybe 'krb5_ctx' is a clearer name?
Yes, it should be.


http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@93
PS2, Line 93:   CHECK_EQ(krb5_get_default_realm(krb5_ctx, _realm), 0);
> What is the performance impact of these extra allocation/deallocations?
It has negative impact on perf. I added this initialization based on comments 
in init.cc InitKrb5Ctx "Work around the lack of thread safety in 
krb5_parse_name() by implicitly
initializing g_krb5_ctx->default_realm once."


http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@97
PS2, Line 97:   krb5_error_code code = krb5_parse_name(krb5_ctx, 
principal.c_str(), );
> You could join with the next line:
Done


http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@99
PS2, Line 99:   *realm = princ->realm.data;
> Is there a test that generates TErrorCode::BAD_PRINCIPAL_FORMAT?
No, I will add one.


http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@100
PS2, Line 100:   krb5_data* data = princ->data;
> Maybe need to call
Done


http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@105
PS2, Line 105:   krb5_free_principal(krb5_ctx, princ);
> I think you can say
Good to know!


http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@111
PS2, Line 111:
> *hostname = data->data;
Done


http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@114
PS2, Line 114:
> Maybe need to call
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e64ebdc10f102dbdc5b87f6fe3f2a0310b1be24
Gerrit-Change-Number: 14433
Gerrit-PatchSet: 3
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Xiaomeng Zhang 
Gerrit-Comment-Date: Mon, 21 Oct 2019 20:14:12 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7504 ParseKerberosPrincipal() should use krb5 parse name() instead

[Xiaomeng Zhang (Code Review)]

Xiaomeng Zhang has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/14433 )

Change subject: IMPALA-7504 ParseKerberosPrincipal() should use 
krb5_parse_name() instead
..

IMPALA-7504 ParseKerberosPrincipal() should use krb5_parse_name() instead

Change-Id: I0e64ebdc10f102dbdc5b87f6fe3f2a0310b1be24
---
M be/src/util/auth-util.cc
1 file changed, 17 insertions(+), 12 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/33/14433/3
--
To view, visit http://gerrit.cloudera.org:8080/14433
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0e64ebdc10f102dbdc5b87f6fe3f2a0310b1be24
Gerrit-Change-Number: 14433
Gerrit-PatchSet: 3
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 

[Impala-ASF-CR] IMPALA-7504 ParseKerberosPrincipal() should use krb5 parse name() instead

[Andrew Sherman (Code Review)]

Andrew Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14433 )

Change subject: IMPALA-7504 ParseKerberosPrincipal() should use 
krb5_parse_name() instead
..


Patch Set 2:

(10 comments)

Thanks for a first cut at this.
I'm still not 100% convinced that this is the right thing to do. but whatever 
happens it is a good training exercise.
I think the general questions are:
What is the performance impact of this change. Is this code on the main path of 
a query?
Is the new function better at dealing with bad input than the old function?

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

http://gerrit.cloudera.org:8080/#/c/14433/2//COMMIT_MSG@8
PS2, Line 8:
If you look at other Impala commit message you will see that they usually try 
to explain what is being done in the commit in some detail. I always imagine I 
am explaining what I am doing to some other developer. This is a place to 
discuss why as well as what you are doing, and any tradeoffs that were 
considered.

You should also add a section explaining what testing you did


http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc
File be/src/util/auth-util.cc:

http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@90
PS2, Line 90:   krb5_context g_krb5_ctx;
Maybe 'krb5_ctx' is a clearer name?


http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@91
PS2, Line 91:   CHECK_EQ(krb5_init_context(_krb5_ctx), 0);
Do you need unused_realm or to call krb5_get_default_realm?


http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@93
PS2, Line 93:   CHECK_EQ(krb5_get_default_realm(g_krb5_ctx, _realm), 0);
What is the performance impact of these extra allocation/deallocations?


http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@97
PS2, Line 97:   krb5_error_code code;
You could join with the next line:
krb5_error_code code = ...


http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@99
PS2, Line 99:   if(code != 0) {
Is there a test that generates TErrorCode::BAD_PRINCIPAL_FORMAT?


http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@100
PS2, Line 100: return Status(TErrorCode::BAD_PRINCIPAL_FORMAT, principal);
Maybe need to call
krb5_free_context()
before returning


http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@105
PS2, Line 105:   char* p = data->data;
I think you can say
*service_name = data->data;


http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@111
PS2, Line 111:   for(int j = 0; j < data->length; j++) {
*hostname = data->data;


http://gerrit.cloudera.org:8080/#/c/14433/2/be/src/util/auth-util.cc@114
PS2, Line 114:   krb5_free_principal(g_krb5_ctx, princ);
Maybe need to call
krb5_free_context()
before returning



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e64ebdc10f102dbdc5b87f6fe3f2a0310b1be24
Gerrit-Change-Number: 14433
Gerrit-PatchSet: 2
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Comment-Date: Tue, 15 Oct 2019 23:16:35 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7504 ParseKerberosPrincipal() should use krb5 parse name() instead

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/14433 )

Change subject: IMPALA-7504 ParseKerberosPrincipal() should use 
krb5_parse_name() instead
..


Patch Set 1:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/4793/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e64ebdc10f102dbdc5b87f6fe3f2a0310b1be24
Gerrit-Change-Number: 14433
Gerrit-PatchSet: 1
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Comment-Date: Tue, 15 Oct 2019 00:34:19 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7504 ParseKerberosPrincipal() should use krb5 parse name() instead

[Xiaomeng Zhang (Code Review)]

Hello Michael Ho, Andrew Sherman, Impala Public Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/14433

to look at the new patch set (#2).

Change subject: IMPALA-7504 ParseKerberosPrincipal() should use 
krb5_parse_name() instead
..

IMPALA-7504 ParseKerberosPrincipal() should use krb5_parse_name() instead

Change-Id: I0e64ebdc10f102dbdc5b87f6fe3f2a0310b1be24
---
M be/src/util/auth-util.cc
1 file changed, 24 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/33/14433/2
--
To view, visit http://gerrit.cloudera.org:8080/14433
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0e64ebdc10f102dbdc5b87f6fe3f2a0310b1be24
Gerrit-Change-Number: 14433
Gerrit-PatchSet: 2
Gerrit-Owner: Xiaomeng Zhang 
Gerrit-Reviewer: Andrew Sherman 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho