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.

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

Update exception handling in PFX.verifyAuthSafes() #1002

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

edewata
Copy link
Contributor

@edewata edewata commented Apr 4, 2024

Previously the PFX.verifyAuthSafes() discarded the original exception and simply returned a generic error message which was usually insufficient to determine the cause of the issue.

To help troubleshooting, the PFX.verifyAuthSafes() has been modified to throw the exception so the original error message and the stack trace are preserved.

@edewata edewata requested a review from fmarco76 April 4, 2024 23:13
Copy link
Member

@fmarco76 fmarco76 left a comment

Choose a reason for hiding this comment

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

This code is invoked by the CLI so if there is an error the user get the stacktrace. I think the original approach with a reason message was better. Could we just add the exception message to the reason to make clearer what is the problem? Is it possible to add more information when the CLI run in debug mode?

reason.append("An exception occurred converting the password from chars to bytes");
return false;
} catch (Exception e) {
throw e;
Copy link
Member

Choose a reason for hiding this comment

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

Since we are not doing any action with the exception then try/catch could be totally removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, the try-catch is removed now.

Previously the PFX.verifyAuthSafes() discarded the original
exception and simply returned a generic error message which
was usually insufficient to determine the cause of the issue.

To help troubleshooting, the PFX.verifyAuthSafes() has been
modified to throw the exception so the original error message
and the stack trace are preserved.
Copy link

sonarqubecloud bot commented Apr 8, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
E Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@edewata
Copy link
Contributor Author

edewata commented Apr 8, 2024

@fmarco76 Please see the updated patch. I removed the try-catch statement as you suggested. As for the stack trace, I think it's better to let the application (i.e. CLI or UI) handle the exception. Currently by default the CLI will only show the exception message without the stack trace, for example:

$ pki pkcs12-cert-find --pkcs12 certs.p12 --password wrong
ERROR: Unable to validate PKCS #12 file: Digests do not match

However, for troubleshooting we can run the CLI in verbose mode to see the stack trace:

$ pki pkcs12-cert-find --pkcs12 certs.p12 --password wrong -v
INFO: Initializing NSS
INFO: Using internal token
java.lang.Exception: Unable to validate PKCS #12 file: Digests do not match
	at org.mozilla.jss.netscape.security.pkcs.PKCS12Util.loadFromByteArray(PKCS12Util.java:830)
	at org.mozilla.jss.netscape.security.pkcs.PKCS12Util.loadFromFile(PKCS12Util.java:816)
	at com.netscape.cmstools.pkcs12.PKCS12CertFindCLI.execute(PKCS12CertFindCLI.java:128)
	at org.dogtagpki.cli.CommandCLI.execute(CommandCLI.java:58)
	at org.dogtagpki.cli.CLI.execute(CLI.java:353)
	at org.dogtagpki.cli.CLI.execute(CLI.java:353)
	at org.dogtagpki.cli.CLI.execute(CLI.java:353)
	at com.netscape.cmstools.cli.MainCLI.execute(MainCLI.java:659)
	at com.netscape.cmstools.cli.MainCLI.main(MainCLI.java:698)

The original code removed the exception info completely so it's not possible to see the stack trace at all, and the original error messages were too generic to be useful.

Copy link
Member

@fmarco76 fmarco76 left a comment

Choose a reason for hiding this comment

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

LGTM

@edewata
Copy link
Contributor Author

edewata commented Apr 8, 2024

@fmarco76 Thanks!

@edewata edewata merged commit 92aa62c into dogtagpki:master Apr 8, 2024
33 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants