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

feat: [Destinations] Support Zero Trust Identity Service #332

Merged
merged 26 commits into from
Mar 28, 2024
Merged

Conversation

MatKuhr
Copy link
Member

@MatKuhr MatKuhr commented Feb 27, 2024

Context

SAP/cloud-sdk-java-backlog#376.

This PR introduces support for the Zero Trust Identity Service and the related credential type X509_ATTESTED in IAS service bindings.

API Design

  • Dedicated Module
    • (+) Can be referenced by connectivity-oauth or other potentially modules as <optional> dependency
    • (+) Using <optional> means no new dedicated public API / class-loading is needed
    • (+) Doesn't come in by default, so doesn't "pollute" the dependency tree of apps that don't need it
    • (+) We can include the linux-specific dependency as users who need this module will almost definitely need it
    • (-) It may be tempting for future developers to simply add this dependency in other places of the SDK, without the optional tag
      • I tried adding an enforcer rule but the enforcer plugin does not seem capable enough to cover this. I tried using banTransitiveDependencies but that wasn't working (at all, really, it bans any transitive dependency and configuring it not to is at a pain at best, I can't get it to work reliably)
  • KeyStore ZeroTrustIdentityService.getInstance().getOrCreateKeyStore()
    • (+) Easy to use
    • (+) Doesn't require Spiffe library in public API
    • (-) It's hard for the user to know when the KeyStore will expire
  • Fallback to file system for local testing by customizing the service binding
    • (+) Based on fixed properties -> SDK doesn't have to do any guessing / fallback
    • (-) Properties are completely "made up", users have to mock the binding locally
    • (-) Mocking bindings locally is super clunky

E2E Test PR: link
Successful E2E test run: link.
Documentation: PR

Feature scope:

  • Load certificates via SPIFFE workload API
  • Fallback to file system access for local testing
  • Integration into OAuth flows with credential type X509_ATTESTED

Definition of Done

  • Functionality scope stated & covered
  • Tests cover the scope above
  • Error handling created / updated & covered by the tests above
  • Documentation updated
  • Release notes updated

@MatKuhr MatKuhr added the please review Request to review a pull request label Mar 22, 2024
@MatKuhr MatKuhr added the please merge Request to merge a pull request label Mar 22, 2024
@MatKuhr MatKuhr marked this pull request as ready for review March 25, 2024 08:08
Copy link
Contributor

@newtork newtork left a comment

Choose a reason for hiding this comment

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

Looks LGTM to me.

@MatKuhr MatKuhr merged commit d2fb4fe into main Mar 28, 2024
16 checks passed
@MatKuhr MatKuhr deleted the feat/zero-trust branch March 28, 2024 12:05
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.

2 participants