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

OAuth2 authentication #48

Open
bmassemin opened this issue Sep 28, 2024 · 10 comments
Open

OAuth2 authentication #48

bmassemin opened this issue Sep 28, 2024 · 10 comments
Labels
enhancement New feature or request

Comments

@bmassemin
Copy link

What would you like?

Be able to authenticate with a service account through OAuth2 client: https://github.com/ovh/go-ovh/blob/master/README.md#oauth2

Contributing

Vote on this issue by adding a 👍 reaction.
To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

@bmassemin bmassemin added the enhancement New feature or request label Sep 28, 2024
@aureq
Copy link
Owner

aureq commented Sep 28, 2024

Hi @bmassemin Thank you for the great suggestion. That sounds like a great idea.

As this feature is more geared toward enterprise, it would be great to either have some form of sponsorship (especially if someone if using that webhook at part of their work) or someone from the community is willing to propose a pull request.

@bmassemin
Copy link
Author

I can handle this myself, but would you prefer using an enum for the auth type (application or oauth2) or a simple oauth2: true/false? Thanks a lot!

@aureq
Copy link
Owner

aureq commented Sep 29, 2024

Are talking about the setting in the Helm Chart under each issuer? Maybe if you could show how the values.yaml would look like, that would be great.

@bmassemin
Copy link
Author

Yes I forgot to mention the Helm Chart.

This is what we have right now:

    # ovhAuthentication:
    #   # the OVH application key. Leave emtpy if you are using an existing secret.
    #   applicationKey: ''
    #   # the OVH application secret. Leave emtpy if you are using an existing secret.
    #   applicationSecret: ''
    #   # Your OVH consumer key. Leave emtpy if you are using an existing secret.
    #   consumerKey: ''

Option 1:

    # ovhAuthentication:
    #   # Authentication method (possible values: application or oauth2)
    #   authenticationMethod: application
    #   # the OVH client ID. Leave emtpy if you are using an existing secret.
    #   clientID: ''
    #   # the OVH client secret. Leave emtpy if you are using an existing secret.
    #   clientSecret: ''
    #   # the OVH application key. Leave emtpy if you are using an existing secret.
    #   applicationKey: ''
    #   # the OVH application secret. Leave emtpy if you are using an existing secret.
    #   applicationSecret: ''
    #   # Your OVH consumer key. Leave emtpy if you are using an existing secret.
    #   consumerKey: ''

Option 2:

    # ovhAuthentication:
    #   # Authentication method (possible values: application or oauth2)
    #   oauth2: false
    #   # the OVH client ID. Leave emtpy if you are using an existing secret.
    #   clientID: ''
    #   # the OVH client secret. Leave emtpy if you are using an existing secret.
    #   clientSecret: ''
    #   # the OVH application key. Leave emtpy if you are using an existing secret.
    #   applicationKey: ''
    #   # the OVH application secret. Leave emtpy if you are using an existing secret.
    #   applicationSecret: ''
    #   # Your OVH consumer key. Leave emtpy if you are using an existing secret.
    #   consumerKey: ''

Option 3: ?

@aureq
Copy link
Owner

aureq commented Oct 1, 2024

Nice!
I definitely prefer option 1 but probably with a very minor tweak.
I think (but let me know what you think about it) having a prefix makes it clearer which key belong to which authentication method.

    ovhAuthentication:
      # Authentication method (possible values: application or oauth2)
      authenticationMethod: application
      # the OVH OAuth 2 client ID. Leave empty if you are using an existing secret.
      oauth2ClientID: ''
      # the OVH OAuth 2 client secret. Leave empty if you are using an existing secret.
      oauth2ClientSecret: ''
      # the OVH application key. Leave empty if you are using an existing secret.
      applicationKey: ''
      # the OVH application secret. Leave empty if you are using an existing secret.
      applicationSecret: ''
      # Your OVH consumer key. Leave empty if you are using an existing secret.
      applicationConsumerKey: ''

@bmassemin
Copy link
Author

I wanted to reuse the same keywords as the OVH configuration for authentication, but it doesn't matter, we can add the oauth2 prefix.

@aureq
Copy link
Owner

aureq commented Oct 1, 2024

Agree that would have been ideal but in this context user experience matters too. That's also why I suggested renaming consumerKey to applicationConsumerKey so the experience is more consistent between the 2 authentication methods.

@bmassemin
Copy link
Author

Ok, but renaming consumerKey introduces a breaking change, we can do that instead:

    ovhAuthentication:
      # Authentication method (possible values: application or oauth2)
      authenticationMethod: application
      # the OVH OAuth 2 client ID. Leave empty if you are using an existing secret.
      oauth2ClientID: ''
      # the OVH OAuth 2 client secret. Leave empty if you are using an existing secret.
      oauth2ClientSecret: ''
      # the OVH application key. Leave empty if you are using an existing secret.
      applicationKey: ''
      # the OVH application secret. Leave empty if you are using an existing secret.
      applicationSecret: ''
      # (DEPRECATED, please use `applicationConsumerKey` instead) Your OVH consumer key. Leave empty if you are using an existing secret.
      consumerKey: ''
      # Your OVH consumer key. Leave empty if you are using an existing secret.
      applicationConsumerKey: ''

@aureq
Copy link
Owner

aureq commented Oct 1, 2024

In that case, I'm ok with a breaking change since we're still in 0.x rather than 1.x for that chart/app. That's also why I recently introduced configVersion in values.yaml so it's easy to flag breaking changes to people.

@bmassemin
Copy link
Author

Ok I'll start the work tomorrow!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants