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

feat(admin): add optional basic auth to admin router #3097

Closed
wants to merge 2 commits into from

Conversation

jimeh
Copy link

@jimeh jimeh commented Apr 28, 2022

Add optional Basic Auth support for securing the Admin API.

Related issue(s)

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security. vulnerability, I
    confirm that I got green light (please contact
    [email protected]) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

This was put together rather quickly, but seems to work nicely. We definitely didn't wrap our heads around Cypress properly for this, so chances are testing is not up to scratch, and we're happy to address any feedback.

Documentation update was also left out, as it looks like docs now live in their own repo.

@jimeh jimeh requested a review from aeneasr as a code owner April 28, 2022 01:25
@CLAassistant
Copy link

CLAassistant commented Apr 28, 2022

CLA assistant check
All committers have signed the CLA.

@@ -6,6 +6,21 @@ describe('The Clients Admin Interface', function () {
grant_types: ['client_credentials']
})

if (Cypress.env('admin_basic_auth')) {
it('should return a 403 error if basic auth credentials are not provided', function () {
Copy link
Author

Choose a reason for hiding this comment

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

Suggested change
it('should return a 403 error if basic auth credentials are not provided', function () {
it('should return a 401 error if basic auth credentials are not provided', function () {

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.

Thank you for this PR and your hard work! Unfortunately, I don't think that we will merge this upstream. Having absolute freedom around how you protect the Admin API is one of the core design principles in Ory. By adding Basic Auth we introduce a pattern of access control which will need to be expanded very soon as use cases arise. For example, someone wants to have expiring and rotating secrets. Someone else wants permissions for clients, the next person mTLS, and so on.

To still improve the state of this we should instead add good documentation and examples (using e.g. Ory Oathkeeper) that explain how to easily protect the admin API of Ory Hydra.

In the future it would be great to create a design issue first to test the waters for larger features such as this one with core maintainers.

Thank you! :)

@aeneasr aeneasr closed this Apr 28, 2022
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.

None yet

4 participants