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

[MJARSIGNER-74] Allow usage of multiple Time Stamping Authority (TSA) servers #19

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

schedin
Copy link
Contributor

@schedin schedin commented Dec 23, 2023

Implementation of support for multiple TSA servers. A new server will be tried if the jarsigner command fails. The failure could be with communication with the TSA server, or could be something unrelated to TSA (for example access to a network based PKSC11 keystore).

I have also implemented support for:

  1. Specifying multiple TSA keystore aliases as an alternative to specifying TSA URLs directly
  2. Specify tsapolicyid OID(s) to send to the TSA server(s)
  3. Specify the message digest algorithm (tsadigestalg) to use in communication with the TSA server (no multi-support, only one value for all TSA servers).

The hardest thing working with this ticket was to figure out what should be possible to configure from an end user perspective. This is what I came up with (and would like feedback on):

  1. I did not see any need to throw an exception (abort the Mojo) if the end user configured something that I consider "wrong". Instead I have logged warnings when this happens
  2. Setting both tsa and tsacert seems wrong. At least jarsigner will ignore -tsacert if -tsa is set. However from a bigger perspective it might be possible for the end user to want to configure 1 TSA url and 2 keystore alias as to try 3 TSA servers in total? But this scenario felt too complicated to document and specify, so I skipped it.
  3. As I tested this feature against public TSA servers it became clear that they will typically have a dedicated, vendor specific OID. For example http://timestamp.digicert.com will only accept the DigiCert created OID 2.16.840.1.114412.7.1. I therefore tried to document that the number of OIDs specified by the end user should be the same as the number of TSA servers the user wants to try.
  4. For tsadigestalg on the other hand I did not see any need to specify multiple. All TSA servers will handle the most common message digest algorithms.
  5. The usage of tsacert, tsapolicyid and tsadigestalg is probably obscure. I'm not sure anybody is interested in using these features. It took me many hours before I could create a testing keystore that contained a valid certificate to use. And it took me some additions hours to figure out what OIDs to use. It was only after using Wireshark to sniff the traffic that I understood how the protocol in RFC 3161 worked.

Note: this pull request can not be merged until a new release build of https://github.com/apache/maven-jarsigner has been made.

… servers

Using multiple TSA URLs to try if first fail
Adding support for tsapolicyid and tsadigestalg
@slawekjaranowski slawekjaranowski merged commit 5535caa into apache:master Sep 3, 2024
20 checks passed
private final String tsaUrl;
private final String tsaAlias;
private final String tsaPolicyId;
private final String tsaDigestAlt;
Copy link

Choose a reason for hiding this comment

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

should it be tsaDigestAlg?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, yes it should be tsaDigestAlg!

Copy link
Member

Choose a reason for hiding this comment

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

@schedin @pzygielo what's now ... should I cancel release vote

Copy link
Member

Choose a reason for hiding this comment

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

we have:

        request.setTsadigestalg(tsaServer.getTsaDigestAlt());

Copy link

Choose a reason for hiding this comment

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

I think it's very internal usage of package-visible only class. No need to cancel vote, as this is not exposed anywhere.

This might be updated anytime later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have made pull request #38 for a spelling error fix. It can be merged at a later time.

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.

3 participants