Update SSLManager.java #654
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1) Added rule for "z/OS" to force validation of password for keystore to be present (ie we always have set a password in z/OS) 2) method getPassword() incorrectly gives warning regarding no password provided, when running without GUI and password is actually fine. ie seems to be a coding error on placement of else statement
ham1
reviewed
Mar 29, 2021
Made update as suggestion
Testing only
|
Dunno why Travis CI failed. |
|
It looks like Daylight Saving Time problem. |
|
Can you give more information on your real problem? Why should z/OS be different in handling non-GUI case than other operating systems? |
|
OK.
I found the real issue I was trying to fix, is that the error message is
not correct, whenever the keyring is supplied, but has no valid key in it
(only certs).
ie It complains about no keyring being supplied, rather than no valid key
in keyring.
This is minor, but leads to confusion when trying to resolve the issue.
It looked to me that the associated } was in wrong place, which caused the
misleading error message to occur.
I figured it out only by writing a stub program that drives the exact code
that parses the keyring file, and prints out it's findings.
There I could see it was skipping the cert entries.
I think for Z/OS we probably have to import a PKCS12 file for the system to
actually import as a key, and not a cert.
This part is our internal issue, as our security team will not give us a
PKCS12 file to import.
I gave up on this change, as I cannot get a successful test until our
security team changes their policies.
Cheers, Russell
*Russell Shaw*
Software Engineer - Career
Acro Database Support
Equifax Inc.
O 770-740-6243
C 678-243-8579
***@***.***
…On Tue, Aug 24, 2021 at 3:49 AM Felix Schumacher ***@***.***> wrote:
Can you give more information on your real problem? Why should z/OS be
different in handling non-GUI case than other operating systems?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<https://protect2.fireeye.com/v1/url?k=92622450-cdf91c80-92620e86-86b9155280ac-3752cdb69b5bc7dd&q=1&e=9fdabfa9-f0e0-483f-a9ab-036c5dc9d6e9&u=https%3A%2F%2Fgithub.com%2Fapache%2Fjmeter%2Fpull%2F654%23issuecomment-904406726>,
or unsubscribe
<https://protect2.fireeye.com/v1/url?k=947b2050-cbe01880-947b0a86-86b9155280ac-7602bc8f6f742a15&q=1&e=9fdabfa9-f0e0-483f-a9ab-036c5dc9d6e9&u=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAQUUKHVD6HYK64D2ZIZN5L3T6NFGPANCNFSM42AIPDDA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://protect2.fireeye.com/v1/url?k=6b7d18f4-34e62024-6b7d3222-86b9155280ac-f145ba0dbd725861&q=1&e=9fdabfa9-f0e0-483f-a9ab-036c5dc9d6e9&u=https%3A%2F%2Fplay.google.com%2Fstore%2Fapps%2Fdetails%3Fid%3Dcom.github.android%26utm_campaign%3Dnotification-email>
.
--
This message contains proprietary information from Equifax which may be
confidential. If you are not an intended recipient, please refrain from any
disclosure, copying, distribution or use of this information and note that
such actions are prohibited. If you have received this transmission in
error, please notify by ***@***.***
***@***.***>. Equifax® is a registered trademark of
Equifax Inc. All rights reserved.
|
|
Also, Z/OS doesn't allow empty password in it's keyrings, and I was trying
to remove the no password supplied error that comes out, when there is no
key in the keyring
*Russell Shaw*
Software Engineer - Career
Acro Database Support
Equifax Inc.
O 770-740-6243
C 678-243-8579
***@***.***
On Tue, Aug 24, 2021 at 1:55 PM Russell Shaw ***@***.***>
wrote:
… OK.
I found the real issue I was trying to fix, is that the error message is
not correct, whenever the keyring is supplied, but has no valid key in it
(only certs).
ie It complains about no keyring being supplied, rather than no valid key
in keyring.
This is minor, but leads to confusion when trying to resolve the issue.
It looked to me that the associated } was in wrong place, which caused the
misleading error message to occur.
I figured it out only by writing a stub program that drives the exact code
that parses the keyring file, and prints out it's findings.
There I could see it was skipping the cert entries.
I think for Z/OS we probably have to import a PKCS12 file for the system
to actually import as a key, and not a cert.
This part is our internal issue, as our security team will not give us a
PKCS12 file to import.
I gave up on this change, as I cannot get a successful test until our
security team changes their policies.
Cheers, Russell
*Russell Shaw*
Software Engineer - Career
Acro Database Support
Equifax Inc.
O 770-740-6243
C 678-243-8579
***@***.***
On Tue, Aug 24, 2021 at 3:49 AM Felix Schumacher ***@***.***>
wrote:
> Can you give more information on your real problem? Why should z/OS be
> different in handling non-GUI case than other operating systems?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://protect2.fireeye.com/v1/url?k=92622450-cdf91c80-92620e86-86b9155280ac-3752cdb69b5bc7dd&q=1&e=9fdabfa9-f0e0-483f-a9ab-036c5dc9d6e9&u=https%3A%2F%2Fgithub.com%2Fapache%2Fjmeter%2Fpull%2F654%23issuecomment-904406726>,
> or unsubscribe
> <https://protect2.fireeye.com/v1/url?k=947b2050-cbe01880-947b0a86-86b9155280ac-7602bc8f6f742a15&q=1&e=9fdabfa9-f0e0-483f-a9ab-036c5dc9d6e9&u=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAQUUKHVD6HYK64D2ZIZN5L3T6NFGPANCNFSM42AIPDDA>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://protect2.fireeye.com/v1/url?k=6b7d18f4-34e62024-6b7d3222-86b9155280ac-f145ba0dbd725861&q=1&e=9fdabfa9-f0e0-483f-a9ab-036c5dc9d6e9&u=https%3A%2F%2Fplay.google.com%2Fstore%2Fapps%2Fdetails%3Fid%3Dcom.github.android%26utm_campaign%3Dnotification-email>
> .
>
--
This message contains proprietary information from Equifax which may be
confidential. If you are not an intended recipient, please refrain from any
disclosure, copying, distribution or use of this information and note that
such actions are prohibited. If you have received this transmission in
error, please notify by ***@***.***
***@***.***>. Equifax® is a registered trademark of
Equifax Inc. All rights reserved.
|
|
Correction on last comment, I was trying to force the no password as error,
when no password supplied for Z/OS, as on Z/OS is is mandatory.
But in the end the root cause was the imported key was imported as a cert,
and resulting messages just confusing
*Russell Shaw*
Software Engineer - Career
Acro Database Support
Equifax Inc.
O 770-740-6243
C 678-243-8579
***@***.***
On Tue, Aug 24, 2021 at 1:58 PM Russell Shaw ***@***.***>
wrote:
… Also, Z/OS doesn't allow empty password in it's keyrings, and I was trying
to remove the no password supplied error that comes out, when there is no
key in the keyring
*Russell Shaw*
Software Engineer - Career
Acro Database Support
Equifax Inc.
O 770-740-6243
C 678-243-8579
***@***.***
On Tue, Aug 24, 2021 at 1:55 PM Russell Shaw ***@***.***>
wrote:
> OK.
>
> I found the real issue I was trying to fix, is that the error message is
> not correct, whenever the keyring is supplied, but has no valid key in it
> (only certs).
> ie It complains about no keyring being supplied, rather than no valid key
> in keyring.
> This is minor, but leads to confusion when trying to resolve the issue.
> It looked to me that the associated } was in wrong place, which caused
> the misleading error message to occur.
>
> I figured it out only by writing a stub program that drives the exact
> code that parses the keyring file, and prints out it's findings.
> There I could see it was skipping the cert entries.
>
> I think for Z/OS we probably have to import a PKCS12 file for the system
> to actually import as a key, and not a cert.
> This part is our internal issue, as our security team will not give us a
> PKCS12 file to import.
> I gave up on this change, as I cannot get a successful test until our
> security team changes their policies.
>
> Cheers, Russell
>
> *Russell Shaw*
> Software Engineer - Career
>
> Acro Database Support
> Equifax Inc.
>
> O 770-740-6243
>
> C 678-243-8579
> ***@***.***
>
>
> On Tue, Aug 24, 2021 at 3:49 AM Felix Schumacher <
> ***@***.***> wrote:
>
>> Can you give more information on your real problem? Why should z/OS be
>> different in handling non-GUI case than other operating systems?
>>
>> —
>> You are receiving this because you authored the thread.
>> Reply to this email directly, view it on GitHub
>> <https://protect2.fireeye.com/v1/url?k=92622450-cdf91c80-92620e86-86b9155280ac-3752cdb69b5bc7dd&q=1&e=9fdabfa9-f0e0-483f-a9ab-036c5dc9d6e9&u=https%3A%2F%2Fgithub.com%2Fapache%2Fjmeter%2Fpull%2F654%23issuecomment-904406726>,
>> or unsubscribe
>> <https://protect2.fireeye.com/v1/url?k=947b2050-cbe01880-947b0a86-86b9155280ac-7602bc8f6f742a15&q=1&e=9fdabfa9-f0e0-483f-a9ab-036c5dc9d6e9&u=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAQUUKHVD6HYK64D2ZIZN5L3T6NFGPANCNFSM42AIPDDA>
>> .
>> Triage notifications on the go with GitHub Mobile for iOS
>> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
>> or Android
>> <https://protect2.fireeye.com/v1/url?k=6b7d18f4-34e62024-6b7d3222-86b9155280ac-f145ba0dbd725861&q=1&e=9fdabfa9-f0e0-483f-a9ab-036c5dc9d6e9&u=https%3A%2F%2Fplay.google.com%2Fstore%2Fapps%2Fdetails%3Fid%3Dcom.github.android%26utm_campaign%3Dnotification-email>
>> .
>>
>
--
This message contains proprietary information from Equifax which may be
confidential. If you are not an intended recipient, please refrain from any
disclosure, copying, distribution or use of this information and note that
such actions are prohibited. If you have received this transmission in
error, please notify by ***@***.***
***@***.***>. Equifax® is a registered trademark of
Equifax Inc. All rights reserved.
|
|
So, the real problem here is, that we currently don't distinguish between no key and wrong password, right? |
|
Right. I supply the keyring file and password in the options file, but it
keeps saying no password supplied (instead of no valid key exists in
keyring)
Initially I thought the options file was not working when loading the
password (invalid character set or something)
We currently read from JKS file.
On Z/OS, password is mandatory for JKS, so I was just trying to have an
earlier error, if password really wasn't supplied in the options file (eg
"changeit")
I was trying to limit my 'fix' to Z/OS just to not adversely impact others.
But that is very trivial, and maybe not needed at all if the subsequent
message was clearer.
Cheers, Russell
*Russell Shaw*
Software Engineer - Career
Acro Database Support
Equifax Inc.
O 770-740-6243
C 678-243-8579
***@***.***
…On Sun, Aug 29, 2021 at 5:16 AM Felix Schumacher ***@***.***> wrote:
So, the real problem here is, that we currently don't distinguish between
no key and wrong password, right?
Which keystore type do you use?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<https://protect2.fireeye.com/v1/url?k=da707e25-85eb4720-da7054f3-86a279d0e4b2-42b764192a8b63be&q=1&e=c1ed756e-efd4-4b60-9596-aebcfef0e089&u=https%3A%2F%2Fgithub.com%2Fapache%2Fjmeter%2Fpull%2F654%23issuecomment-907757944>,
or unsubscribe
<https://protect2.fireeye.com/v1/url?k=5c7f3dc2-03e404c7-5c7f1714-86a279d0e4b2-08e53882d69a57bd&q=1&e=c1ed756e-efd4-4b60-9596-aebcfef0e089&u=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAQUUKHRSXQEO5BVSYWN2HCLT7H3H3ANCNFSM42AIPDDA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://protect2.fireeye.com/v1/url?k=cbd15ed7-944a67d2-cbd17401-86a279d0e4b2-c35d20998852061a&q=1&e=c1ed756e-efd4-4b60-9596-aebcfef0e089&u=https%3A%2F%2Fplay.google.com%2Fstore%2Fapps%2Fdetails%3Fid%3Dcom.github.android%26referrer%3Dutm_campaign%253Dnotification-email%2526utm_medium%253Demail%2526utm_source%253Dgithub>.
--
This message contains proprietary information from Equifax which may be
confidential. If you are not an intended recipient, please refrain from any
disclosure, copying, distribution or use of this information and note that
such actions are prohibited. If you have received this transmission in
error, please notify by ***@***.***
***@***.***>. Equifax® is a registered trademark of
Equifax Inc. All rights reserved.
|
asfgit
pushed a commit
that referenced
this pull request
Sep 11, 2021
Currently a user can not differentiate between a wrong password and missing or wrong keys, algorithms, etc. So try do handle the excpetions, that the API gives us and log different error messages to better inform our users. Part of #654
|
I have tried to differentiate the exceptions a bit more. Could you those changes out and report back, whether they would have helped you? |
|
This seems to have been done differently in: Closing |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fix erroneous warning message output when loading client cert from keystore with password in non-GUI mode, when password is actually present.
Add extra condition for z/OS operating system, so warning message will appear if keystore password is ever null.
Code changes made are :
This will then trigger the warning message to appear if defaultpw = null.
Motivation and Context
Misleading error messages that confused debugging issues around keystore loading and jmeter configuration files
How Has This Been Tested?
Unit tested in own environment.
Types of changes
Checklist: