Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Modify Shell to authenticate user on call to config #3440

Merged
merged 9 commits into from
Jun 8, 2023
Merged

Modify Shell to authenticate user on call to config #3440

merged 9 commits into from
Jun 8, 2023

Conversation

dlmarion
Copy link
Contributor

No description provided.

@dlmarion dlmarion requested a review from ctubbsii May 30, 2023 18:06
@dlmarion dlmarion self-assigned this May 30, 2023
@dlmarion dlmarion linked an issue May 30, 2023 that may be closed by this pull request
Always authenticate users in the shell after startup and when switching users #3433
Closed
@cshannon
Copy link
Contributor

cshannon commented May 31, 2023 •
edited
Loading

The failure appears to be related to the CRYPT_PASSWORD_CACHE inside of ZKSecurityTool. I commented out this line and the test no longer fails:

accumulo/server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKSecurityTool.java

Line 152 in 969fc52

CRYPT_PASSWORD_CACHE.put(key, cryptHash);

@dlmarion dlmarion marked this pull request as ready for review June 7, 2023 20:49
@dlmarion
Copy link
Contributor Author

dlmarion commented Jun 7, 2023

Findbugs is complaining about:

Medium: Hard coded password found [org.apache.accumulo.shell.Shell, org.apache.accumulo.shell.Shell, org.apache.accumulo.shell.Shell, org.apache.accumulo.shell.ShellOptionsJC] At Shell.java:[line 283]At Shell.java:[line 370]At Shell.java:[line 371]At ShellOptionsJC.java:[line 148] HARD_CODE_PASSWORD

Not sure what the issue is, there is no hardcoded password

EdColeman
EdColeman reviewed Jun 7, 2023
View reviewed changes
Copy link
Contributor

@EdColeman EdColeman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only findbug warning I received running locally were ignoring the return value from delete(file) - I added them as suggestions - but the formatting may be off.

@dlmarion
Merge branch '2.1' into 3433-always-authenticate-shell-user
5b30cfb
@dlmarion
Changes to resolve FindBugs issues
8f71e5c 
Check config.delete return value
Rename variable from password to authenticationString
@dlmarion
Copy link
Contributor Author

dlmarion commented Jun 8, 2023

It looks like changing the variable name from password to something that the findbugs password checking plugins are not looking for enables the build to pass. My guess is that it thought that the password was hardcoded in ShellOptionsJC and then highlighted everywhere it was referenced.

@ctubbsii
Code review from ctubbsii for apache#3440
2db232f 
* Use protected method and override in test instead of injecting static
  implementation as a public static field that allows any thread to
  modify the shell's authentication behavior
* Restore previous context-specific prompts by adding passwordPrompt
  string to the getAuthenticationToken method
* Tweak token variable name to minimize diff and for formatting
* Use existing client config file in ShellAuthenticatorIT
ctubbsii
ctubbsii approved these changes Jun 8, 2023
View reviewed changes
Copy link
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a commit 2db232f to reflect my code review suggested changes. It can be rolled back out in part or in whole if needed, but this seemed like the most efficient way to review.

With those changes, I think this looks good to me.

@dlmarion
Copy link
Contributor Author

dlmarion commented Jun 8, 2023

No issue here.

@dlmarion
Copy link
Contributor Author

dlmarion commented Jun 8, 2023

@ctubbsii - It looks like you reintroduced the sec-bugs issue:

Medium: Hard coded password found [org.apache.accumulo.shell.Shell, org.apache.accumulo.shell.Shell, org.apache.accumulo.shell.Shell, org.apache.accumulo.shell.ShellOptionsJC] At Shell.java:[line 276]At Shell.java:[line 363]At Shell.java:[line 364]At ShellOptionsJC.java:[line 148] HARD_CODE_PASSWORD

Some of the sec-bugs password plugins contain patterns for variable names. That's why I chose to use a different variable name.

@ctubbsii
Restore variable rename workaround for spotbugs
56c2101 
* Use the authenticationString variable name instead of password to
  prevent spotbugs from detecting it as a false positive hard-coded
  password
* This restores the previous commit that did this, which got clobbered
  when I was resolving conflicts with my code review commit (sorry!)
@ctubbsii
Copy link
Member

ctubbsii commented Jun 8, 2023

@ctubbsii - It looks like you reintroduced the sec-bugs issue:

Yep, sorry about that. I missed that one when I was resolving conflicts with my code review commit. Should be fixed now.

@dlmarion dlmarion merged commit 0f23897 into apache:2.1 Jun 8, 2023
@dlmarion dlmarion deleted the 3433-always-authenticate-shell-user branch June 8, 2023 20:46
@ctubbsii ctubbsii added this to the 2.1.1 milestone Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EdColeman EdColeman EdColeman left review comments

@ctubbsii ctubbsii ctubbsii approved these changes

@keith-turner keith-turner Awaiting requested review from keith-turner

Assignees

@dlmarion dlmarion

Labels
None yet
Projects
None yet
Milestone
2.1.1
Development

Successfully merging this pull request may close these issues.

Always authenticate users in the shell after startup and when switching users
5 participants
@dlmarion @cshannon @ctubbsii @keith-turner @EdColeman