这是indexloc提供的服务，不要输入任何密码
Skip to content

#4736 Improvements to auth testing #4919

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jun 11, 2020
Merged

#4736 Improvements to auth testing #4919

merged 4 commits into from
Jun 11, 2020

Conversation

@jberryman
Copy link
Collaborator

@jberryman jberryman commented May 28, 2020 •
edited
Loading

Description

One minor user-facing change. The second commit is just a refactor for ease of unit testing (and as a consequence, better code organization hopefully)

Changelog

  • CHANGELOG.md is updated with user-facing content relevant to this PR.

Affected components

  • Server
  • Docs
  • Community Content
  • Tests

Related Issues

#4736 ... it's not clear if this is sufficient to close that issue

Solution and Design

Simple factoring out of tricky bits to make unit tests of core auth logic possible. This would also be the first step towards more clever solutions like e.g. having a JWT transformer/effect, but I don't think that gives us much.

Steps to test and verify

  • Please read through test spec carefully, make sure actual behavior is desired
  • make note of change mentioned in changelog
  • look closely at big block moves (with poor diff; sorry about that), to make sure they are in fact trivial refactors

Limitations, known bugs & workarounds

Server checklist

Catalog upgrade

Does this PR change Hasura Catalog version?

  • No
  • Yes
    • Updated docs with SQL for downgrading the catalog

Metadata

Does this PR add a new Metadata feature?

  • No
  • Yes
    • Does run_sql auto manages the new metadata through schema diffing?
      • Yes
      • Not required
    • Does run_sql auto manages the definitions of metadata on renaming?
      • Yes
      • Not required
    • Does export_metadata/replace_metadata supports the new metadata added?
      • Yes
      • Not required

GraphQL

  • No new GraphQL schema is generated
  • New GraphQL schema is being generated:
    • New types and typenames are correlated

Breaking changes

  • No Breaking changes

  • There are breaking changes:

    1. Metadata API

      Existing query types:

      • Modify args payload which is not backward compatible
      • Behavioural change of the API
      • Change in response JSON schema
      • Change in error code

    2. GraphQL API

      Schema Generation:

      • Change in any NamedType
      • Change in table field names

      Schema Resolve:-

      • Change in treatment of null value for any input fields

    3. Logging

      • Log JSON schema has changed
      • Log type names have changed

@netlify
Copy link

netlify bot commented May 28, 2020 •
edited
Loading

Deploy preview for hasura-docs ready!

Built with commit d862afb

https://deploy-preview-4919--hasura-docs.netlify.app

@hasura-bot
Copy link
Contributor

hasura-bot commented May 28, 2020

Review app for commit 85a4d1b deployed to Heroku: https://hge-ci-pull-4919.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4919-85a4d1b5

@jberryman jberryman force-pushed the 4736-security-testing branch from 85a4d1b to a8bcd7b Compare May 28, 2020 18:04
@hasura-bot
Copy link
Contributor

hasura-bot commented May 28, 2020

Review app for commit a8bcd7b deployed to Heroku: https://hge-ci-pull-4919.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4919-a8bcd7bb

@jberryman jberryman force-pushed the 4736-security-testing branch from a8bcd7b to fa2589f Compare May 28, 2020 18:37
@jberryman jberryman requested review from 0x777 and rakeshkky May 28, 2020 18:40
@jberryman jberryman self-assigned this May 28, 2020
@jberryman jberryman linked an issue May 28, 2020 that may be closed by this pull request
Improvements to Hasura security testing #4736
Closed
@jberryman jberryman marked this pull request as ready for review May 28, 2020 18:41
@jberryman jberryman requested review from a team as code owners May 28, 2020 18:41
@hasura-bot
Copy link
Contributor

hasura-bot commented May 28, 2020

Review app for commit fa2589f deployed to Heroku: https://hge-ci-pull-4919.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4919-fa2589fc

@jberryman
Copy link
Collaborator Author

jberryman commented Jun 2, 2020

@0x777 @rakeshkky bump for review

@abooij abooij self-requested a review June 4, 2020 14:37
abooij
abooij suggested changes Jun 5, 2020
View reviewed changes
Copy link
Contributor

@abooij abooij left a comment

Choose a reason for hiding this comment

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

I've gone through the code, and most of my feedback relates to the comments, namely the motivation of the code. I think this PR improves on the quality of the authentication and of the testing that we do, and so my feedback is just an attempt to help it one small step further.

I should add that I can't say that I'm convinced that the authentication code is correct. We'd need to significantly rework how authentication is done, so that more correctness properties can be obvious from the type system alone, rather than the right checks and guards being in place. It should be possible to write an API which encapsulates authentication using compile-time guarantees, and additionally works by a "whitelisting" approach rather than blacklisting access using ifs and cases. But I won't ask this to be done now.

@0x777 I did my best to review this, but for such an important topic I'd like your input on this as well, at least to judge if you think there are any weak spots in my analysis.

jberryman added 2 commits June 8, 2020 13:09
@jberryman
Tighten up handling of admin secret, more docs
d747bc1 
Store the admin secret only as a hash to prevent leaking the secret
inadvertently, and to prevent timing attacks on the secret.

NOTE: best practice for stored user passwords is a function with a
tunable cost like bcrypt, but our threat model is quite different (even
if we thought we could reasonably protect the secret from an attacker
who could read arbitrary regions of memory), and bcrypt is far too slow
(by design) to perform on each request. We'd have to rely on our
(technically savvy) users to choose high entropy passwords in any case.

Referencing hasura#4736
@jberryman
Refactor and unit test authentication code paths (closes hasura#4736)
5e37350 
The bulk of changes here is some shifting of code around and a little
parameterizing of functions for easier testing.

Also: comments, some renaming for clarity/less-chance-for-misue.
@jberryman jberryman force-pushed the 4736-security-testing branch from fa2589f to 5e37350 Compare June 8, 2020 17:11
@jberryman
Copy link
Collaborator Author

jberryman commented Jun 8, 2020

Thanks for the review @abooij ! Addressed comments in latest

@hasura-bot
Copy link
Contributor

hasura-bot commented Jun 8, 2020

Review app for commit 5e37350 deployed to Heroku: https://hge-ci-pull-4919.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4919-5e373505

abooij
abooij reviewed Jun 9, 2020
View reviewed changes
Copy link
Contributor

@abooij abooij left a comment

Choose a reason for hiding this comment

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

(If changes are added as new commits, rather than as a new force-pushed changeset, reviewers can easily see how their comments have been addressed.)

@0x777, could you give a final approval of this?

0x777
0x777 approved these changes Jun 11, 2020
View reviewed changes
Copy link
Member

@0x777 0x777 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Changelog LGTM

@hasura-bot
Copy link
Contributor

hasura-bot commented Jun 11, 2020

Review app for commit d862afb deployed to Heroku: https://hge-ci-pull-4919.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4919-d862afb4

@0x777 0x777 merged commit 4c9362c into hasura:master Jun 11, 2020
@hasura-bot
Copy link
Contributor

hasura-bot commented Jun 11, 2020

Review app https://hge-ci-pull-4919.herokuapp.com is deleted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@0x777 0x777 0x777 approved these changes

@rakeshkky rakeshkky Awaiting requested review from rakeshkky

+2 more reviewers

@abooij abooij abooij requested changes

@marionschleifer marionschleifer marionschleifer approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jberryman jberryman

Labels

no-changelog-required

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Improvements to Hasura security testing

5 participants

@jberryman @hasura-bot @abooij @marionschleifer @0x777