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.

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

Cert auth #223

Merged
merged 40 commits into from
Jan 18, 2024
Merged

Cert auth #223

merged 40 commits into from
Jan 18, 2024

Conversation

jhazentia
Copy link
Member

@jhazentia jhazentia commented Jan 30, 2023 •
edited
Loading

Replaces OAuth/JWT authentication with certificate based authentication for authentication to sensor and to callback URL

  • Browsable api timeout, logout doesn't work, always logged in
  • Admin interface still uses username/password
  • Updated README with some updates specific to this pull request and some more general updates
  • Added ADDITIONAL_USER_NAMES and ADDITIONAL_USER_PASSWORD environment variables to programmatically create additional users when scos-sensor starts. ADDITIONAL_USER_NAMES can be comma separated. ADDITIONAL_USER_PASSWORD is not required even when using ADDITIONAL_USER_NAMES. However, without a password, the user will not be able to log in with session authentication or to the admin interface. However, certificate and token authentication will still work.
  • Added ADMIN_NAME environment variable to control username for admin account
  • Removed environment variables related to OAuth: CLIENT_ID, CLIENT_SECRET, OAUTH_TOKEN_URL, PATH_TO_JWT_PUBLIC_KEY, USER_NAME, PASSWORD
  • Removed SENTRY_DSN environment variable
  • Modified NGINX config for ssl client verification and to pass the certificate DN in request header
  • Removed restore_fixture.sh
  • Removed role based permissions; require all users to be superusers
  • Added tests

dboulware and others added 30 commits July 29, 2022 09:28
@dboulware
Initial implementation of certificate rest api authentication
06f9965
@dboulware
Use env variable to set admin user name.
5664774
@dboulware
Add ADMIN_NAME setting to be able to pass CN of admin user do db crea…
c0e2574 
…tion. Add setting for NGINX to set DN.
@jhazentia
raise exception if user not found, remove token auth and session auth
04c1f55
@jhazentia
Merge branch 'master' of https://github.com/NTIA/scos-sensor into cer…
adbdd0b 
…t_auth
@jhazentia
cert auth in migration and runtime settings
16b3f05
@jhazentia
update requirements
9cb4c80
@jhazentia
remove pyjwt from requirements
ba037ab
@jhazentia
update readme
a7d5291
@jhazentia
remove jwt/oauth, change permission to IsSuperuser
8b9d253
@jhazentia
fix tests for cert auth, remove oauth variables from settings, fix sc…
dfc673d 
…heduler cert auth callback
@jhazentia
update README, remove unused files, fix tests, cleanup, exception han…
d36ce6b 
…dling
@jhazentia
update openapi security definitions, autoformatting, settings comment
1846c2c
@jhazentia
remove oauth/jwt from isort; autoformat settings, openapi.json
6b21fc6
@jhazentia
remove old oauth params
89a3b48
@jhazentia
remove login url when using cert auth
6d1a256
@jhazentia
update last_login, nginx check ssl_client_verify
a5fd9de
@jhazentia
Merge branch 'master' of https://github.com/NTIA/scos-sensor into cer…
a423430 
…t_auth
@jhazentia
fix merge issue for requirements-dev.in
fcbc88e
@jhazentia
fix comment
bfcc86c
@jhazentia
Merge branch 'master' of https://github.com/NTIA/scos-sensor into cer…
41bfa03 
…t_auth
@jhazentia
update authentication information in readme
7409e93
@jhazentia
keep logout url for page timeout
761bcdc
@jhazentia
Merge branch 'master' of https://github.com/NTIA/scos-sensor into cer…
32a340c 
…t_auth
@jhazentia
update dependencies, fix dependabot alert
38d369f
@jhazentia
formatting updates
a04ea35
@jhazentia
Update env.template
457e38c 
remove SENTRY_DSN which is no longer used
@jhazentia
Merge branch 'master' of https://github.com/NTIA/scos-sensor into cer…
f54010b 
…t_auth
@jhazentia
dependency updates
8850365
@jhazentia
support multiple users in create_superuser, fix get username from cer…
740c89a 
…t, additional tests, update readme
@jhazentia jhazentia marked this pull request as ready for review December 4, 2023 20:02
dboulware
dboulware requested changes Jan 5, 2024
View reviewed changes
Copy link
Contributor

@dboulware dboulware left a comment

Choose a reason for hiding this comment

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

This looks good. Only some minor comments to address.

README.md Outdated Show resolved Hide resolved
README.md Outdated

###### Step 3: Extract Private Key
The Nginx web server is configured by default to require client certificates (mutual
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it has changed. I suggest editing this to show how to change it to enable cert auth.

Copy link
Member Author

Choose a reason for hiding this comment

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

I made some changes. Please check if new version is good (commit e9b2f30)

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed ssl_ocsp on setting from README Nginx configuration text blocks. This has not been tested and unsure if additional settings required.

README.md Show resolved Hide resolved
src/authentication/auth.py Show resolved Hide resolved
scripts/create_superuser.py Outdated Show resolved Hide resolved
nginx/conf.template Outdated Show resolved Hide resolved
@jhazentia jhazentia requested a review from dboulware January 8, 2024 19:56
dboulware
dboulware requested changes Jan 16, 2024
View reviewed changes
Copy link
Contributor

@dboulware dboulware left a comment

Choose a reason for hiding this comment

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

Looks good. Just one minor request. Add to the log message the user that could not be found when unable to find the user.

@jhazentia jhazentia requested a review from dboulware January 16, 2024 17:21
@dboulware dboulware self-requested a review January 17, 2024 16:45
dboulware
dboulware requested changes Jan 17, 2024
View reviewed changes
Copy link
Contributor

@dboulware dboulware left a comment

Choose a reason for hiding this comment

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

Looks good. Can you resolve the conflicts?

@jhazentia
Copy link
Member Author

Looks good. Can you resolve the conflicts?

Done

@jhazentia jhazentia requested a review from dboulware January 17, 2024 18:05
@dboulware dboulware merged commit 9c7a857 into master Jan 18, 2024
4 checks passed
@dboulware dboulware deleted the cert_auth branch February 29, 2024 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dboulware dboulware dboulware approved these changes

@cheroy-ntia cheroy-ntia Awaiting requested review from cheroy-ntia

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jhazentia @dboulware