-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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.
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
8279064: New options for ktab to provide non-default salt #6991
Conversation
8279064: New options for ktab to provide non-default salt
/csr |
👋 Welcome back weijun! A progress list of the required criteria for merging this PR into |
Webrevs
|
I will take a look. Thanks~ |
I added myself to the CSR as reviewer. Made minor edits, re-ordered the command line option ordering based on the ordering mentioned in the description. |
src/java.security.jgss/windows/classes/sun/security/krb5/internal/tools/Ktab.java
Outdated
Show resolved
Hide resolved
src/java.security.jgss/windows/classes/sun/security/krb5/internal/tools/Ktab.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks~
/integrate auto |
@wangweij This pull request will be automatically integrated when it is ready |
@wangweij This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 131 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
/integrate |
2 similar comments
/integrate |
/integrate |
Going to push as commit 0d1a97f.
Your commit was automatically rebased without conflicts. |
@openjdk[bot] The command |
@openjdk[bot] The command |
Please review this enhancement and its CSR. Two new options
-s salt
and-f
can be specified on thektab
command when adding entries.I'm a little concerned about the compatibility risk described in the CSR, i.e. the
-f
option is already used inktab -d
to force removing entries. Hopefully not many people are writing their own wrappers on ktab that always include the-f
option. I do want to be consistent with the naming in the MIT krb5 ktutil command.Another thing worth mentioning is the KerberosKey:<new>(KerberosPrincipal principal, char[] password, String algorithm) constructor which always uses the default salt. For consistency, it looks like a new constructor should be added that takes the salt string as a parameter as well. However, I don't intend to add it as I cannot see a proper usage for it. In fact, I now regret adding the constructor linked above.
Progress
Issues
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6991/head:pull/6991
$ git checkout pull/6991
Update a local copy of the PR:
$ git checkout pull/6991
$ git pull https://git.openjdk.java.net/jdk pull/6991/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 6991
View PR using the GUI difftool:
$ git pr show -t 6991
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6991.diff