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

Allow cred/authorization header CORS #101

Merged
merged 7 commits into from
Nov 10, 2021

Conversation

fredx30
Copy link
Contributor

@fredx30 fredx30 commented Nov 1, 2021

Summary

This adds a environment variable that allows setting a cors allowed origin. When this is set Authrization Bearer tokens will also be allowed to be sent. This overrides the allow all origins header. When sending credentials allow all origins is not allowed.

Motivation

When sending credentials allow all origins is not allowed. The way the OICD login is set to work credentials need to be sent and will use the authorization header.

@fredx30 fredx30 self-assigned this Nov 1, 2021
@fredx30 fredx30 added the enhancement New feature or request label Nov 1, 2021
@fredx30 fredx30 added this to In progress in Backlog via automation Nov 1, 2021
@fredx30 fredx30 linked an issue Nov 1, 2021 that may be closed by this pull request
@fredx30 fredx30 marked this pull request as ready for review November 1, 2021 11:43
CHANGELOG.md Outdated Show resolved Hide resolved
config.go Outdated Show resolved Hide resolved
config.go Outdated
// results in the HTTP header "AllowOrigins".
//
// Added in v5.0.0.
AllowOrigins string
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be a slice of strings. While the header only can include 1 value, IIRC Gin will check the Origin header from the request and use the appropriate origin in the response header.

While we probably won't need more than 1, it's good to have the option available, especially since it's such an easy change but hard to change later as it changes the config schema.

Apparently Viper (that we're using for the configs) supports slices by separating elements via a comma.
(https://github.com/spf13/viper/blob/v1.7.1/viper.go#L920)

So you can change this to []string and then add something like this to the godoc:

Multiple origins can be supplied via environment variables by separating them by commas. YAML configuration is expected to use a regular array of strings.

main.go Outdated
@@ -110,7 +110,14 @@ func main() {
ginutil.RecoverProblem,
)

if config.HTTP.CORS.AllowAllOrigins {
if len(config.HTTP.CORS.AllowOrigins) > 0 {
log.Info().Message("Allowing " + config.HTTP.CORS.AllowOrigins + " origins in CORS.")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: A rule of thumb I've used for logs is to not have anything varying in the message.

Suggest moving the config value here to a WithString call. For example:

Suggested change
log.Info().Message("Allowing " + config.HTTP.CORS.AllowOrigins + " origins in CORS.")
log.Info().WithString("origin", config.HTTP.CORS.AllowOrigins).
Message("Allowing origins in CORS.")

But if you're changing to a string slice, then perhaps just something like this:

Suggested change
log.Info().Message("Allowing " + config.HTTP.CORS.AllowOrigins + " origins in CORS.")
log.Info().WithStringf("origin", "%v", config.HTTP.CORS.AllowOrigins).
Message("Allowing origins in CORS.")

^ would resolve to something like origins=`[http://localhost:5001 http://localhost:4200]` in the console output for multiple values

Otherwise a more custom solution:

Suggested change
log.Info().Message("Allowing " + config.HTTP.CORS.AllowOrigins + " origins in CORS.")
log.Info().WithString("origin", strings.Join(config.HTTP.CORS.AllowOrigins, " ")).
Message("Allowing origins in CORS.")

^ would resolve to something like origins=`http://localhost:5001 http://localhost:4200` in the console output for multiple values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sol 2 seems to work

CHANGELOG.md Outdated Show resolved Hide resolved
Backlog automation moved this from In progress to Review in progress Nov 1, 2021
@fredx30 fredx30 mentioned this pull request Nov 8, 2021
2 tasks
@fredx30 fredx30 force-pushed the feature/cors-credentials-allow-origin-specific branch from 3dea685 to 49fb077 Compare November 8, 2021 21:04
Copy link
Member

@Alexamakans Alexamakans left a comment

Choose a reason for hiding this comment

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

I agree that AllowOrigins should be []string, with updated godoc.
Prefer opt 2 for logging, resulting in origins=`[http://localhost:5001 http://localhost:4200]`

Will approve once AllowOrigins is changed to []string 👍🏻

@applejag applejag force-pushed the feature/cors-credentials-allow-origin-specific branch from b5ca077 to ed95856 Compare November 9, 2021 15:36
Copy link
Contributor

@applejag applejag left a comment

Choose a reason for hiding this comment

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

LGTM!

Backlog automation moved this from Review in progress to Reviewer approved Nov 9, 2021
@fredx30 fredx30 force-pushed the feature/cors-credentials-allow-origin-specific branch from ed95856 to 25e4e8b Compare November 10, 2021 09:50
@fredx30 fredx30 merged commit 3a0c8e8 into master Nov 10, 2021
Backlog automation moved this from Reviewer approved to Done Nov 10, 2021
@fredx30 fredx30 deleted the feature/cors-credentials-allow-origin-specific branch November 10, 2021 10:00
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
No open projects
Development

Successfully merging this pull request may close these issues.

Authentication, first iteration
3 participants