feat: Add OAuth support for HTTP Sync#1791
Conversation
✅ Deploy Preview for polite-licorice-3db33c canceled.
|
There was a problem hiding this comment.
Summary of Changes
Hello @thisthat, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces OAuth 2.0 client credentials flow support for HTTP synchronization, allowing flagd to authenticate with remote flag sources using OAuth. This enhances security and flexibility for fetching flag configurations.
Highlights
- OAuth Integration: Implemented OAuth 2.0 client credentials flow for HTTP Sync, enabling secure authentication with flag sources.
- Configurable HTTP Client: The HTTP client used for synchronization now dynamically configures itself to use OAuth if
oauthConfigis provided in the source configuration. - New Configuration Structure: Introduced a dedicated
OAuthConfigstructure to define OAuth client ID, client secret, and token URL within the source configuration.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
025f17c to
2b4b714
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces OAuth support for the HTTP Sync provider, which is a great addition. The implementation is solid, but I've identified a few areas for improvement to enhance robustness, flexibility, and adherence to Go's idiomatic coding style. Specifically, the new OAuth HTTP client is missing a request timeout, which could lead to hanging requests. Additionally, the OAuth authentication style is hardcoded, which might limit compatibility with some OAuth servers. Lastly, some of the new struct field names could be adjusted to better align with Go conventions. I've provided detailed comments and code suggestions to address these points.
2d884e9 to
4bc7a9a
Compare
| ), nil | ||
| } | ||
|
|
||
| func (sb *SyncBuilder) newHTTP(config sync.SourceConfig, logger *logger.Logger) *httpSync.Sync { |
There was a problem hiding this comment.
move the logic of creating the HTTPSync inside the HTTPSync
| BearerToken string | ||
| AuthHeader string | ||
| Interval uint32 | ||
| uri string |
There was a problem hiding this comment.
we don't need to expose these details anymore since we moved its construction here
aepfli
left a comment
There was a problem hiding this comment.
Generally those changes look great, we do have some minor improvements/questions - but generally the implementation looks fine.
The biggest part missing tough is documentation, and currently this looks like a hidden feature. Especially the fact that filesource for oauth takes priority, and the current double purpose of clientId and clientSecret.
|
I'll review this tomorrow, first thing. Thanks @thisthat |
…2.4.0 [security] (open-feature#1784) This PR contains the following updates: | Package | Change | Age | Confidence | |---|---|---|---| | [github.com/go-viper/mapstructure/v2](https://redirect.github.com/go-viper/mapstructure) | `v2.3.0` -> `v2.4.0` | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | ### GitHub Vulnerability Alerts #### [GHSA-2464-8j7c-4cjm](https://redirect.github.com/go-viper/mapstructure/security/advisories/GHSA-2464-8j7c-4cjm) ### Summary Use of this library in a security-critical context may result in leaking sensitive information, if used to process sensitive fields. ### Details OpenBao (and presumably HashiCorp Vault) have surfaced error messages from `mapstructure` as follows: https://github.com/openbao/openbao/blob/98c3a59c040efca724353ca46ca79bd5cdbab920/sdk/framework/field_data.go#L43-L50 ```go _, _, err := d.getPrimitive(field, schema) if err != nil { return fmt.Errorf("error converting input for field %q: %w", field, err) } ``` where this calls `mapstructure.WeakDecode(...)`: https://github.com/openbao/openbao/blob/98c3a59c040efca724353ca46ca79bd5cdbab920/sdk/framework/field_data.go#L181-L193 ```go func (d *FieldData) getPrimitive(k string, schema *FieldSchema) (interface{}, bool, error) { raw, ok := d.Raw[k] if !ok { return nil, false, nil } switch t := schema.Type; t { case TypeBool: var result bool if err := mapstructure.WeakDecode(raw, &result); err != nil { return nil, false, err } return result, true, nil ``` Notably, `WeakDecode(...)` eventually calls one of the decode helpers, which surfaces the original value via `strconv` helpers: https://github.com/go-viper/mapstructure/blob/8c61ec1924fcfa522f9fc6b4618c672db61d1a38/mapstructure.go#L720-L727 https://github.com/go-viper/mapstructure/blob/8c61ec1924fcfa522f9fc6b4618c672db61d1a38/mapstructure.go#L791-L798 https://github.com/go-viper/mapstructure/blob/8c61ec1924fcfa522f9fc6b4618c672db61d1a38/decode_hooks.go#L180 & more. These are different code paths than are fixed in the previous iteration at GHSA-fv92-fjc5-jj9h. ### PoC To reproduce with OpenBao: ``` $ podman run --pull=always -p 8300:8300 openbao/openbao:latest server -dev -dev-root-token-id=root -dev-listen-address=0.0.0.0:8300 ``` and in a new tab: ``` $ BAO_TOKEN=root BAO_ADDR=http://localhost:8300 bao auth enable userpass Success! Enabled userpass auth method at: userpass/ $ curl -X PUT -H "X-Vault-Request: true" -H "X-Vault-Token: root" -d '{"ttl":"asdf"}' "http://localhost:8200/v1/auth/userpass/users/asdf" --> server logs: 2025-06-25T21:32:25.101-0500 [ERROR] core: failed to run existence check: error="error converting input for field \"ttl\": time: invalid duration \"asdf\"" ``` ### Impact This is an information disclosure bug with little mitigation. See https://discuss.hashicorp.com/t/hcsec-2025-09-vault-may-expose-sensitive-information-in-error-logs-when-processing-malformed-data-with-the-kv-v2-plugin/74717 for a previous version. That version was fixed, but this is in the second part of that error message (starting at `'' expected a map, got 'string'` -- when the field type is `string` and a `map` is provided, we see the above information leak -- the previous example had a `map` type field with a `string` value provided). This was rated 4.5 Medium by HashiCorp in the past iteration. --- ### Release Notes <details> <summary>go-viper/mapstructure (github.com/go-viper/mapstructure/v2)</summary> ### [`v2.4.0`](https://redirect.github.com/go-viper/mapstructure/releases/tag/v2.4.0) [Compare Source](https://redirect.github.com/go-viper/mapstructure/compare/v2.3.0...v2.4.0) #### What's Changed - refactor: replace interface{} with any by [@​sagikazarmark](https://redirect.github.com/sagikazarmark) in [https://github.com/go-viper/mapstructure/pull/115](https://redirect.github.com/go-viper/mapstructure/pull/115) - build(deps): bump github/codeql-action from 3.29.0 to 3.29.2 by [@​dependabot](https://redirect.github.com/dependabot)\[bot] in[https://github.com/go-viper/mapstructure/pull/114](https://redirect.github.com/go-viper/mapstructure/pull/114)4 - Generic tests by [@​sagikazarmark](https://redirect.github.com/sagikazarmark) in [https://github.com/go-viper/mapstructure/pull/118](https://redirect.github.com/go-viper/mapstructure/pull/118) - Fix godoc reference link in README.md by [@​peczenyj](https://redirect.github.com/peczenyj) in [https://github.com/go-viper/mapstructure/pull/107](https://redirect.github.com/go-viper/mapstructure/pull/107) - feat: add StringToTimeLocationHookFunc to convert strings to \*time.Location by [@​ErfanMomeniii](https://redirect.github.com/ErfanMomeniii) in [https://github.com/go-viper/mapstructure/pull/117](https://redirect.github.com/go-viper/mapstructure/pull/117) - feat: add back previous StringToSlice as a weak function by [@​sagikazarmark](https://redirect.github.com/sagikazarmark) in [https://github.com/go-viper/mapstructure/pull/119](https://redirect.github.com/go-viper/mapstructure/pull/119) #### New Contributors - [@​ErfanMomeniii](https://redirect.github.com/ErfanMomeniii) made their first contribution in [https://github.com/go-viper/mapstructure/pull/117](https://redirect.github.com/go-viper/mapstructure/pull/117) **Full Changelog**: go-viper/mapstructure@v2.3.0...v2.4.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR was generated by [Mend Renovate](https://mend.io/renovate/). View the [repository job log](https://developer.mend.io/github/open-feature/flagd). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0MS44MS4yIiwidXBkYXRlZEluVmVyIjoiNDEuODEuMiIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsicmVub3ZhdGUiXX0=--> --------- Signed-off-by: Todd Baert <todd.baert@dynatrace.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Todd Baert <todd.baert@dynatrace.com> Signed-off-by: Giovanni Liva <giovanni.liva@dynatrace.com>
Signed-off-by: Giovanni Liva <giovanni.liva@dynatrace.com>
Signed-off-by: Giovanni Liva <giovanni.liva@dynatrace.com>
Signed-off-by: Giovanni Liva <giovanni.liva@dynatrace.com>
Signed-off-by: Giovanni Liva <giovanni.liva@dynatrace.com>
Signed-off-by: Giovanni Liva <giovanni.liva@dynatrace.com>
Signed-off-by: Giovanni Liva <giovanni.liva@dynatrace.com>
Signed-off-by: Giovanni Liva <giovanni.liva@dynatrace.com>
…-feature#1792) Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com> Signed-off-by: Giovanni Liva <giovanni.liva@dynatrace.com>
Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com> Signed-off-by: Giovanni Liva <giovanni.liva@dynatrace.com>
Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com> Co-authored-by: chrfwow <christian.lutnik@dynatrace.com> Signed-off-by: Giovanni Liva <giovanni.liva@dynatrace.com>
depends: open-feature#1792 for fixing build issues --------- Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com> Signed-off-by: Giovanni Liva <giovanni.liva@dynatrace.com>
Signed-off-by: Giovanni Liva <giovanni.liva@dynatrace.com>
6cab606 to
f83bf93
Compare
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
<!-- Please use this template for your pull request. --> <!-- Please use the sections that you need and delete other sections --> ## This PR - adds docs for #1791 ### Related Issues Fixes #1785 --------- Signed-off-by: Giovanni Liva <giovanni.liva@dynatrace.com> Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com> Signed-off-by: Todd Baert <todd.baert@dynatrace.com> Co-authored-by: Simon Schrottner <simon.schrottner@dynatrace.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: alexandraoberaigner <82218944+alexandraoberaigner@users.noreply.github.com> Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
🤖 I have created a release *beep* *boop* --- <details><summary>flagd: 0.13.0</summary> ## [0.13.0](flagd/v0.12.9...flagd/v0.13.0) (2025-12-23) ### 🐛 Bug Fixes * fixing sync return format missing flag layer, adding full e2e suite ([#1827](#1827)) ([570693d](570693d)) * **security:** update module github.com/go-viper/mapstructure/v2 to v2.4.0 [security] ([#1784](#1784)) ([037e30b](037e30b)) * **security:** update module golang.org/x/crypto to v0.45.0 [security] ([#1826](#1826)) ([7e0762b](7e0762b)) ### ✨ New Features * add support for http-based ofrep metrics ([#1803](#1803)) ([fcd19b3](fcd19b3)) * cleanup evaluator interface ([#1793](#1793)) ([aa504f7](aa504f7)) * enable parsing of array flag configurations for flagd ([#1797](#1797)) ([97c6ffa](97c6ffa)) * multi-project support via selectors and flagSetId namespacing ([#1702](#1702)) ([f9ce46f](f9ce46f)) * normalize selector in sync (use header as in OFREP and RPC) ([#1815](#1815)) ([c1f06cb](c1f06cb)) ### 🧹 Chore * **refactor:** use memdb for flag storage ([#1697](#1697)) ([5c5c1cf](5c5c1cf)) ### 🔄 Refactoring * store cleanup ([#1705](#1705)) ([bcff8d7](bcff8d7)) </details> <details><summary>flagd-proxy: 0.8.1</summary> ## [0.8.1](flagd-proxy/v0.8.0...flagd-proxy/v0.8.1) (2025-12-23) ### 🐛 Bug Fixes * **security:** update module github.com/go-viper/mapstructure/v2 to v2.4.0 [security] ([#1784](#1784)) ([037e30b](037e30b)) * **security:** update module golang.org/x/crypto to v0.45.0 [security] ([#1826](#1826)) ([7e0762b](7e0762b)) </details> <details><summary>core: 0.13.0</summary> ## [0.13.0](core/v0.12.1...core/v0.13.0) (2025-12-23) ### ⚠ BREAKING CHANGES * enable parsing of array flag configurations for flagd ([#1797](#1797)) * cleanup evaluator interface ([#1793](#1793)) * removes the `fractionalEvaluation` operator since it has been replaced with `fractional`. ([#1704](#1704)) ### 🐛 Bug Fixes * **security:** update module github.com/go-viper/mapstructure/v2 to v2.4.0 [security] ([#1784](#1784)) ([037e30b](037e30b)) * **security:** update module golang.org/x/crypto to v0.45.0 [security] ([#1825](#1825)) ([44edcc9](44edcc9)) * **security:** update module golang.org/x/crypto to v0.45.0 [security] ([#1826](#1826)) ([7e0762b](7e0762b)) ### ✨ New Features * Add OAuth support for HTTP Sync ([#1791](#1791)) ([268fd75](268fd75)) * Add OTEL default variables ([#1812](#1812)) ([c2e3fc6](c2e3fc6)) * allow null flagSetId Selector, restrict Selector to single key-value-pairs ([#1708](#1708)) ([#1811](#1811)) ([c12a0ae](c12a0ae)) * change jsonschema parser ([#1794](#1794)) ([bf3f722](bf3f722)) * cleanup evaluator interface ([#1793](#1793)) ([aa504f7](aa504f7)) * enable parsing of array flag configurations for flagd ([#1797](#1797)) ([97c6ffa](97c6ffa)) * multi-project support via selectors and flagSetId namespacing ([#1702](#1702)) ([f9ce46f](f9ce46f)) ### 🧹 Chore * **refactor:** use memdb for flag storage ([#1697](#1697)) ([5c5c1cf](5c5c1cf)) * removes the `fractionalEvaluation` operator since it has been replaced with `fractional`. ([#1704](#1704)) ([3228ad8](3228ad8)) ### 🔄 Refactoring * remove deprecated bearerToken option ([#1816](#1816)) ([efda06a](efda06a)) * removed unused Selector from Flag and Store. ([#1747](#1747)) ([1083005](1083005)) * store cleanup ([#1705](#1705)) ([bcff8d7](bcff8d7)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Signed-off-by: OpenFeature Bot <109696520+openfeaturebot@users.noreply.github.com> Signed-off-by: Todd Baert <todd.baert@dynatrace.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
This PR
Related Issues
Addresses #1785
Notes
Properly testing this via unit tests is not possible. The HTTP Sync is built by the
syncbuilderpkg. This entails that testing any behavior of the HTTP Sync can only be possible as part of thesyncbuildermodule, which IMHO doesn't make much sense.Follow-up Tasks
Add documentation for the new properties used in the HTTP Sync.
Available here: #1805
How to test
Manually create a small mockup server that handles OAuth requests
Afterwards, start flagd pointing to the mockup server:
The HTTP request to
http://localhost:8180/flagswill contain the HTTP HeaderAuthorization: Bearer mocktoken.