8366261: Provide utility methods for sun.security.util.Password #26973

naotoj · 2025-08-27T22:53:28Z

Providing a couple of utility methods using the "built-in" Console implementation to support tools that require password input, such as keytool, ensuring they work even when std0ut is redirected.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8366261: Provide utility methods for sun.security.util.Password (Enhancement - P4)

Reviewers

Stuart Marks (@stuart-marks - Reviewer)
Weijun Wang (@wangweij - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26973/head:pull/26973
$ git checkout pull/26973

Update a local copy of the PR:
$ git checkout pull/26973
$ git pull https://git.openjdk.org/jdk.git pull/26973/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26973

View PR using the GUI difftool:
$ git pr show -t 26973

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26973.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-08-27T22:54:42Z

👋 Welcome back naoto! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-08-27T22:55:36Z

@naotoj 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:

8366261: Provide utility methods for sun.security.util.Password

Reviewed-by: smarks, weijun

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 195 new commits pushed to the master branch:

cc6d34b: 8366971: C2: Remove unused nop_list from PhaseOutput::init_buffer
b653ae9: 8367051: Build failure with clang on linux and AIX after switch to C++17
f10c85f: 8367027: java/lang/ProcessBuilder/Basic.java fails on Windows AArch64
... and 192 more: https://git.openjdk.org/jdk/compare/1ff73cb2ec41612d316921e852f29e7fa4dc9109...master

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 master branch, type /integrate in a new comment.

openjdk · 2025-08-27T22:56:08Z

@naotoj The following labels will be automatically applied to this pull request:

core-libs
security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-08-27T22:59:40Z

Webrevs

liach · 2025-08-27T23:28:15Z

src/java.base/share/classes/java/io/Console.java

-    private static final boolean istty = istty();
+    static final int TTY_STDIN_MASK = 0x00000001;
+    static final int TTY_STDOUT_MASK = 0x00000002;
+    static final int TTY_STDERR_MASK = 0x00000004;


I recommend marking these fields as @Native.

Good point. Fixed

test/jdk/java/io/Console/ModuleSelectionTest.java

liach · 2025-08-28T13:41:47Z

src/java.base/share/classes/jdk/internal/io/JdkConsoleImpl.java

+    public static Optional<JdkConsoleImpl> passwordConsole() {
+        return INSTANCE.orElseSet(() -> {
+            // If there's already a proper console, throw an exception
+            if (System.console() != null) {


Since this initializes the console, this means the console can only be available when there is no provider and stdin is tty, but stdout is not. Is this intended? (In other words, should we make System.console() lazy, as it is initialized with java.io.Console as Console.cons?)

Yes, this is intentional. This entry is dedicated solely to Password, and it is expected that System.console() will be called beforehand so that normal cases (without redirection) are handled by System.console(). The null check is a safeguard to prevent future Password refactoring from invoking this method without first calling System.console() in the standard case.

It would be good to add comments that describes this precondition. We've all been discussing it so we "know" what it's doing and why. But other readers, like @liach, and future maintainers of the Password class, will know what the requirements are. (EDIT: I meant "will need to know" but I think you understood.)

It would be good to add comments that describes this precondition.

Good point. Explained it in its comment.

Co-authored-by: Francesco Andreuzzi <andreuzzi.francesco@gmail.com>

justin-curtis-lu · 2025-08-28T16:24:54Z

test/jdk/java/io/Console/ModuleSelectionTest.java

-/**
+/*
 * @test
 * @bug 8295803 8299689 8351435 8361613


Should 8366261 be included?

stuart-marks · 2025-09-04T23:28:22Z

src/java.base/share/classes/jdk/internal/io/JdkConsoleImpl.java

+                    new JdkConsoleImpl(
+                        Charset.forName(StaticProperty.stdinEncoding(), UTF_8.INSTANCE),
+                        Charset.forName(StaticProperty.stdoutEncoding(), UTF_8.INSTANCE))) :
+                Optional.empty();


(Finally getting back to this.)

The code returns an Optional containing a JdkConsoleImpl instance, or an empty Optional. However the comment above says it returns a JdkConsoleImpl instance or null. Which do we want it to be? Either is OK, but the comment should agree with the code.

I think @wangweij might want to weigh in here since his code will be calling this.

Ah, yes. Comment does not align with the code. I am fine either way too.

Either is OK. Optional sounds more modern and forces me to consider the empty case.

Fixed the comment to agree with the code

stuart-marks

Looks good.

wangweij · 2025-09-09T15:27:20Z

src/java.base/share/classes/jdk/internal/io/JdkConsoleImpl.java

+        });
+    }
+
+    // Dedicated entry for sun.security.util.Password.


To be more clear, I suggest this comment:

// Dedicated entry for sun.security.util.Password when stdout is redirected. // This method strictly avoids producing any output by using noNewLine = true // and an empty format string.

Thanks. Modified the comment as suggested

wangweij · 2025-09-09T17:22:59Z

Thanks for the comment change. Approved.

stuart-marks

Not sure whether I need to re-approve, but I will anyway!

naotoj · 2025-09-09T19:35:33Z

Thanks for the reviews!
/integrate

openjdk · 2025-09-09T19:38:04Z

Going to push as commit a12e9fc.
Since your change was applied there have been 195 commits pushed to the master branch:

cc6d34b: 8366971: C2: Remove unused nop_list from PhaseOutput::init_buffer
b653ae9: 8367051: Build failure with clang on linux and AIX after switch to C++17
f10c85f: 8367027: java/lang/ProcessBuilder/Basic.java fails on Windows AArch64
... and 192 more: https://git.openjdk.org/jdk/compare/1ff73cb2ec41612d316921e852f29e7fa4dc9109...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-09-09T19:38:19Z

@naotoj Pushed as commit a12e9fc.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

naotoj added 12 commits August 18, 2025 17:13

init

96d10e0

static JdkConsoleImpl in JdkConsoleProviderImpl

da109a5

Merge branch 'master' into base-provider-impl

13b7539

int istty

2ffa53e

Removed dedicated impl class

a2a3c30

test fix

77da007

return type: Object->JdkConsoleImpl

bca257c

passing delegate to JdkConsoleImpl

705d4fa

Self-check System.console() availability

eab367d

istty -> ttyStatus, etc

8a88920

Merge branch 'master' into base-provider-impl

2fffa1e

Expose isStdinTty() instead of ttyStatus() through JavaIOAccess

2539387

openjdk bot added security security-dev@openjdk.org core-libs core-libs-dev@openjdk.org rfr Pull request is ready for review labels Aug 27, 2025

liach reviewed Aug 27, 2025

View reviewed changes

@Native annotated

bbfb036

fandreuz reviewed Aug 28, 2025

View reviewed changes

test/jdk/java/io/Console/ModuleSelectionTest.java Outdated Show resolved Hide resolved

liach reviewed Aug 28, 2025

View reviewed changes

Update test/jdk/java/io/Console/ModuleSelectionTest.java

5c2053f

Co-authored-by: Francesco Andreuzzi <andreuzzi.francesco@gmail.com>

justin-curtis-lu reviewed Aug 28, 2025

View reviewed changes

naotoj added 2 commits August 28, 2025 09:41

@bug id

f79c5a0

Refining comments

ff7e9e0

stuart-marks reviewed Sep 4, 2025

View reviewed changes

Fix comments

9306bc0

stuart-marks approved these changes Sep 9, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 9, 2025

wangweij reviewed Sep 9, 2025

View reviewed changes

Comment refined

b928b2c

openjdk bot removed the ready Pull request is ready to be integrated label Sep 9, 2025

wangweij approved these changes Sep 9, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 9, 2025

stuart-marks approved these changes Sep 9, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Sep 9, 2025

openjdk bot closed this Sep 9, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 9, 2025

8366261: Provide utility methods for sun.security.util.Password #26973

8366261: Provide utility methods for sun.security.util.Password #26973

Uh oh!

Conversation

naotoj commented Aug 27, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Aug 27, 2025

Uh oh!

openjdk bot commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Aug 27, 2025

Uh oh!

mlbridge bot commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuart-marks Aug 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuart-marks Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wangweij Sep 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuart-marks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wangweij commented Sep 9, 2025

Uh oh!

stuart-marks left a comment

Choose a reason for hiding this comment

Uh oh!

naotoj commented Sep 9, 2025

Uh oh!

openjdk bot commented Sep 9, 2025

Uh oh!

openjdk bot commented Sep 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

naotoj commented Aug 27, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Aug 27, 2025 •

edited

Loading

mlbridge bot commented Aug 27, 2025 •

edited

Loading

stuart-marks Aug 30, 2025 •

edited

Loading

stuart-marks Sep 4, 2025 •

edited

Loading

wangweij Sep 6, 2025 •

edited

Loading