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

Added graceful downgrade from TLS security enabled to disabled if a TLS CLIENT_KEY is not provided #11

Merged
merged 2 commits into from
Nov 3, 2020
Merged

Added graceful downgrade from TLS security enabled to disabled if a TLS CLIENT_KEY is not provided #11

merged 2 commits into from
Nov 3, 2020

Conversation

adriansuarez
Copy link
Collaborator

No description provided.

mkysel
mkysel reviewed Nov 3, 2020
View reviewed changes
nuocd/nuodb_adminquery.py
# this assumes set to a path. It could be set to True in which case we need
# to return the default path

server_cert = env.get('NUOCMD_VERIFY_SERVER', os.environ.get('NUOCMD_VERIFY_SERVER', False))
Copy link
Contributor

Choose a reason for hiding this comment

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

server_cert can also be missing. In 4.2 we no longer ship either the server cert or the client cert.

A missing server_cert file is "OK" in python?

Copy link
Collaborator Author

@adriansuarez adriansuarez Nov 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we should downgrade if the client key exists. If someone went through the trouble to create nuocmd.pem, but they forgot to create or misplaced (e.g. they called it /etc/nuodb/keys/ca.crt instead of /etc/nuodb/keys/ca.cert) the server certificate, then they would probably be expecting HTTPS to be used.

I'm also being more defensive about downgrading here because I can't tell if the argument was resolved from an environment variable or an explicit argument, (nuocmd only downgrades when the explicit arguments are not specified).

sivanov-nuodb
sivanov-nuodb reviewed Nov 3, 2020
View reviewed changes
Copy link
Collaborator

@sivanov-nuodb sivanov-nuodb left a comment

Choose a reason for hiding this comment

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

I also think that we should check server_cert file existence. Otherwise the changes look good to me.

sivanov-nuodb
sivanov-nuodb approved these changes Nov 3, 2020
View reviewed changes
Copy link
Collaborator

@sivanov-nuodb sivanov-nuodb left a comment

Choose a reason for hiding this comment

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

I guess that the verify certificate file is not used even if provided in case the protocol is downgraded.

@adriansuarez adriansuarez merged commit 4225094 into master Nov 3, 2020
@adriansuarez adriansuarez deleted the asuarez/http-downgrade branch November 3, 2020 17:58
@mkysel mkysel changed the title Downgrade to http:// if key does not exist Added graceful downgrade to API_SERVER http:// if CLIENT_KEY does not exist Nov 4, 2020
@mkysel mkysel changed the title Added graceful downgrade to API_SERVER http:// if CLIENT_KEY does not exist Added graceful downgrade for API_SERVER to http:// if CLIENT_KEY does not exist Nov 4, 2020
@mkysel mkysel added the enhancement New feature or request label Nov 4, 2020
@jleslie85 jleslie85 changed the title Added graceful downgrade for API_SERVER to http:// if CLIENT_KEY does not exist Added graceful downgrade from TLS security enabled to disabled if a TLS CLIENT_KEY is not provided Nov 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkysel mkysel mkysel left review comments

@sivanov-nuodb sivanov-nuodb sivanov-nuodb approved these changes

@butson butson Awaiting requested review from butson

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adriansuarez @mkysel @sivanov-nuodb