-
-
Notifications
You must be signed in to change notification settings - Fork 132
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
relay-proxy: Support API Keys validation to limit the applications being able to use GO Feature Flag #649
relay-proxy: Support API Keys validation to limit the applications being able to use GO Feature Flag #649
Conversation
✅ Deploy Preview for go-feature-flag-doc-preview canceled.
|
Codecov Report
@@ Coverage Diff @@
## main #649 +/- ##
=======================================
Coverage 88.40% 88.40%
=======================================
Files 58 58
Lines 2605 2605
=======================================
Hits 2303 2303
Misses 252 252
Partials 50 50
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
hi @thomaspoignant just drafted this PR for initial feedback please also check the controller part cmd/relayproxy/controller/api_key.go one more thing, currently we can put |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey thanks for this PR.
I am not sure to understand how you proposal will work.
How can I add an API key? By calling the API and it will be stored in memory?
How will it work when the relay-proxy will restart, will we lose the API keys configured?
How I was thinking of doing this, was to have a list of API keys in the relay proxy configuration file and I would have challenged the authorization headers with everything that is part of this list.
using the
yes
yes everything will be lost
got it, let me rework the PR, I'm more aligned this way too, thank you for your feedback! |
…ing able to use GO Feature Flag
…ing able to use GO Feature Flag, rework by feedback
b782d3a
to
bcab9a7
Compare
…ing able to use GO Feature Flag, simplify
…ing able to use GO Feature Flag, update docs
…ing able to use GO Feature Flag, update docs
…ing able to use GO Feature Flag, update swagger
@@ -47,7 +47,7 @@ Before starting your **relay proxy** you will need to create a minimal configura | |||
# this is a minimal config containing only where your flag file is located | |||
retriever: | |||
kind: http | |||
url: https://raw.githubusercontent.com/thomaspoignant/go-feature-flag/main/examples/file/flags.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
invalid url
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch 👍
…ing able to use GO Feature Flag, gen swagger
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach is way closer to what I had in mind thank you 🙏
I've added a few review point because I am not sure to get all the points here.
cmd/relayproxy/config/config.go
Outdated
@@ -137,6 +138,9 @@ type Config struct { | |||
|
|||
// Version is the version of the relay-proxy | |||
Version string | |||
|
|||
// APIKeys list of API keys that authorized to use endpoints | |||
APIKeys map[string]bool `mapstructure:"apiKeys"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure to understand why you have a map[string]bool
, is it to have a set here?
If so we may need to consider having a map[string]interface{}
instead since it is a bit better in term of memory and size.
See https://itnext.io/set-in-go-map-bool-and-map-struct-performance-comparison-5315b4b107b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the article, TIL
let me change it into slice since it's more easy to understand in config
apiKeys: | ||
apikey1: true | ||
apikey2: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure to understand the boolean here.
Would it be better to have only a list, something like
apiKeys:
- apikey1 # with a comment explaining who is the owner of the key
- apikey2
I think it will make the configuration representation way easier to understand.
You can still put it in a set in the config struct, but in terms of configuration, it will be way more understandable for the users of the relay proxy.
@@ -31,6 +31,7 @@ If you want to replace a nested fields, please use `_` to separate each field _( | |||
| `startWithRetrieverError` | boolean | `false` | By default the **relay proxy** will crash if he is not able to retrieve the flags from the configuration.<br/>If you don't want your relay proxy to crash, you can set `startWithRetrieverError` to true. Until the flag is retrievable the relay proxy will only answer with default values. | | |||
| `exporter` | [exporter](#exporter) | **none** | Exporter is the configuration on how to export data. | | |||
| `notifier` | [notifier](#notifier) | **none** | Notifiers is the configuration on where to notify a flag change. | | |||
| `apiKeys` | map[string]bool | **none** | List of authorized API keys. Each request will need to provide one of authorized key inside `Authorization` header with format `Bearer <api-key>`. There will be no authorization when this config is not configured.<br /><br />_Note: there will be no authorization when this config is not set._ | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mention twice this, maybe we should remove one.
There will be no authorization when this config is not configured.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right.
I guess we could delete the one in cmd/relayproxy/docs
since it is not really used and keep only the one that finish on the website.
…ing able to use GO Feature Flag, fix review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks exactly to what I had in mind.
I will review it soon.
Signed-off-by: Thomas Poignant <[email protected]>
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhanusaputra thanks a lot for this pull request this is a super great new addition to GO Feature Flag 🙏
Description
will use
APIKeys
to initiate list of keysconfigurable in goff.yaml
use
middleware.KeyAuthWithConfig
to check the header with format:Bearer <key>
middleware.KeyAuthWithConfig.Validator
will reject unauthorized requestsbasically will add one more process in middleware to check the header
Changes include
Closes issue(s)
Resolve #613
Checklist
README.md
and/website/docs
)