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

Add equals/hashcode for Destination #341

Merged
merged 15 commits into from
Mar 8, 2024

Conversation

newtork
Copy link
Contributor

@newtork newtork commented Mar 5, 2024

Backlog item: https://github.com/SAP/cloud-sdk-java-backlog/issues/403

Context

  • A destination can be enhanced with keyStore and trustStore properties; both of Java type KeyStore. Useful in mTLS scenarios or when having a custom root certificate for TLS/SSL.
  • Java KeyStore API does not offer a custom implementation for equals and hashCode. This is why we exclude it from evaluation in the DefaultHttpDestination class.
  • We primarily use hashCode to isolate destination-related cache entries, e.g. for resolving stored HttpClient instances.

Currently on main.

Destinations are evaluated as equal with same hash-code:

  • Trivial case
    • Destination1(name=foo, staticHeaders={sap-client:100})
    • Destination2(name=foo, staticHeaders={sap-client:100})
  • Assigned KeyStore or TrustStore
    • Destination1(name=foo, staticHeaders={sap-client:100}, keyStore={fizz})
    • Destination2(name=foo, staticHeaders={sap-client:100})
  • Assigned KeyStore mixed TrustStore
    • Destination1(name=foo, staticHeaders={sap-client:100}, keyStore={fizz})
    • Destination2(name=foo, staticHeaders={sap-client:100}, trustStore={buzz})

With this PR

Destinations are evaluated as equal with same hash-code:

  • Trivial case
    • Destination1(name=foo, staticHeaders={sap-client:100})
    • Destination2(name=foo, staticHeaders={sap-client:100})
  • Assigned KeyStore or TrustStore
    • Destination1(name=foo, staticHeaders={sap-client:100}, keyStore={fizz})
    • Destination2(name=foo, staticHeaders={sap-client:100})
  • Assigned KeyStore mixed TrustStore
    • Destination1(name=foo, staticHeaders={sap-client:100}, keyStore={fizz})
    • Destination2(name=foo, staticHeaders={sap-client:100}, trustStore={buzz})

Limitation

This behavior change only applies to KeyStore instances that have certificate based entries only.
E.g. if a KeyStore with only a secret-key would be added - it will be ignored from equals/hashCode, i.e. the previous behavior of main applies again.

@newtork newtork marked this pull request as draft March 5, 2024 09:34
Copy link
Contributor

@CharlesDuboisSAP CharlesDuboisSAP left a comment

Choose a reason for hiding this comment

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

My comments are also valid for resolveKeyStoreHashCode.

}
}
catch( final Exception e ) {
log.debug("Error while resolving certificates from KeyStore", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

It will only throw when a keyStore is not initialized. The first thing in resolveCertificatesOnly should be filtering for un-initialized keystores. Or maybe in the destination itself.

Copy link
Contributor Author

@newtork newtork Mar 7, 2024

Choose a reason for hiding this comment

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

  • There is no method on KeyStore to check for "is-loaded" unfortunately.
  • The existing API and behavior assumes the KeyStore in keyStore and trustStore fields are already loaded. There is no "lazy loading" or sth like that in already initialized Destination / HttpClient objects.
  • I want to avoid additional logic within DefaultHttpDestination class or its super classes as far as reasonably possible, this is why I put all potential logic into this helper comparator util class.

if( !ks.isCertificateEntry(alias) ) {
return new Certificate[0];
}
out.add(ks.getCertificate(alias));
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if a certificate is followed by "not a certificate"? Should "not a certificate" rather be ignored instead?

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 didn't want to mix behavior:
As soon as a non-certificate based entry occurs, the logic falls back to current main behavior, i.e. ignore everything.

@newtork newtork marked this pull request as ready for review March 8, 2024 08:54
@newtork newtork added please merge Request to merge a pull request please review Request to review a pull request labels Mar 8, 2024
Copy link
Contributor

@CharlesDuboisSAP CharlesDuboisSAP left a comment

Choose a reason for hiding this comment

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

I hope those destinations are supposed to be equal:

final KeyStore keyStore = KeyStore.getInstance("JKS");
keyStore.load(null);
keyStore.setCertificateEntry("a", cert);
Destination dest = DefaultHttpDestination.builder("https://example.com").keyStore(keyStore).build();
final KeyStore keyStore2 = KeyStore.getInstance("JKS");
keyStore2.load(null);
keyStore2.setCertificateEntry("b", cert);
Destination dest2 = DefaultHttpDestination.builder("https://example.com").keyStore(keyStore2).build();
assertThat(dest).isEqualTo(dest2);

Otherwise LGTM

@MatKuhr
Copy link
Member

MatKuhr commented Mar 8, 2024

I hope those destinations are supposed to be equal:

final KeyStore keyStore = KeyStore.getInstance("JKS");
keyStore.load(null);
keyStore.setCertificateEntry("a", cert);
Destination dest = DefaultHttpDestination.builder("https://example.com").keyStore(keyStore).build();
final KeyStore keyStore2 = KeyStore.getInstance("JKS");
keyStore2.load(null);
keyStore2.setCertificateEntry("b", cert);
Destination dest2 = DefaultHttpDestination.builder("https://example.com").keyStore(keyStore2).build();
assertThat(dest).isEqualTo(dest2);

Otherwise LGTM

I think this is fine, currently the aliases don't matter AFAIK. I think it wouldn't hurt to also compare the aliases, they should always be equal as well in our usage, but not super required I guess..

@newtork
Copy link
Contributor Author

newtork commented Mar 8, 2024

Good find, @CharlesDuboisSAP. I haven't noticed that I removed the alias comparison :/
This was indeed unintended change during my commits.

@MatKuhr is right, the aliases are not important at runtime.

However not comparing them might be add to the confusion 🤔 On the other hand adding new logic now to compare Enumeration<String> aliases (does not implement equals/hashCode natively) would add unnecessary code complexity and potential edge cases that we don't want to cover explicitly (yet).

Therefore I'll leave it as is.
Sorry future devs ;)

@newtork newtork merged commit 37be46d into main Mar 8, 2024
16 checks passed
@newtork newtork deleted the destination-keystore-equals-hashcode branch March 8, 2024 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
please merge Request to merge a pull request please review Request to review a pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants