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

Headers Credentials Issuer #100

Merged
merged 10 commits into from
Aug 16, 2018
Merged

Headers Credentials Issuer #100

merged 10 commits into from
Aug 16, 2018

Conversation

zikes
Copy link
Contributor

@zikes zikes commented Aug 9, 2018

Implemented as discussed in #96

Usage:

{
    "id": "some-id",
    "upstream": {"url": "http://my-backend-service"},
    "match": { },
    "authenticators": [ ],
    "authorizer": { },
    "credentials_issuer": {
        "handler": "headers",
        "config": {
            "X-User": "{{ print .Subject }}",
            "X-Audience": "{{ print .Extra.aud }}",
            "X-Issuer": "{{ print .Extra.iss }}",
            "X-Arbitrary": "{{ print .Extra.some.deeply.nested.value }}"
        }
    }
}

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

This is awesome, very clean and short and tests look good too. Only two things from my side. And a third one: docs!

Could you please make a PR and add this CI to the rules documentation?

Thank you!

}

var cfg CredentialsHeadersConfig
d := json.NewDecoder(bytes.NewBuffer(config))
Copy link
Member

Choose a reason for hiding this comment

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

I think it would make sense here to have d.DisallowUnknownFields() to complain on potential typos.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The CredentialsHeadersConfig is just a map[string]string so there are no unknown fields, the map keys correspond to HTTP headers so they have to remain freeform. Thinking about it now, though, I realize that may constrain future configuration options. I could turn it into a struct with a Headers map[string]string field instead, which would go from

"credentials_issuer": {
    "handler": "headers",
    "config": {
        "X-User": "{{ print .Subject }}",
        "X-Audience": "{{ print .Extra.aud }}"
    }
}

to

"credentials_issuer": {
    "handler": "headers",
    "config": {
        "headers": {
            "X-User": "{{ print .Subject }}",
            "X-Audience": "{{ print .Extra.aud }}"
        }
    }
}

But that may end up being an unnecessary change if we never introduce any new configuration options for that issuer. I can't think of anything I could add, though. I'll let you decide which you prefer and implement as directed.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, it could be possible that we want to configure the headers handler further (maybe add a filter:true/false option which removes unused X- headers?). I think it's a good idea to separate the template from potential future updates to the config. It will improve BC as well!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👌

var err error

templateId := fmt.Sprintf("%s:%s", rl.ID, hdr)
tmpl = a.RulesCache.Lookup(templateId)
Copy link
Member

Choose a reason for hiding this comment

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

Is RulesCache thread-safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, it's a text.Template and they have a RWMutex guarding parsing and executing: https://github.com/golang/go/blob/master/src/text/template/template.go#L20

Copy link
Member

Choose a reason for hiding this comment

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

Ok cool!

@aeneasr
Copy link
Member

aeneasr commented Aug 16, 2018

Thank you for your hard work!

@aeneasr aeneasr merged commit 585672e into ory:master Aug 16, 2018
NickUfer pushed a commit to NickUfer/oathkeeper that referenced this pull request Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants