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

Require authentication by default #4059

Merged
merged 3 commits into from
May 22, 2024
Merged

Require authentication by default #4059

merged 3 commits into from
May 22, 2024

Conversation

gguillemas
Copy link
Contributor

@gguillemas gguillemas commented May 17, 2024

Thank you for submitting this pull request! We really appreciate you spending the time to work on these changes.

What is the motivation?

To prevent SurrealDB users from accidentally allowing unauthenticated access to their server. This is specially relevant for users running SurrealDB in cloud instances with publicly addressable network interfaces.

What does this change do?

Replaces the --auth flag with --unauthenticated, which defaults to false. If no flag is provided, the server will run with authentication and a message will no longer be printed announcing it to the user. If the --unauthenticated flag is provided, the server will run without requiring authentication and a warning will be printed.

What is your testing strategy?

Ensure that existing tests pass.

Is this related to any issues?

No.

Does this change need documentation?

Yes, this needs an update on the CLI documentation page.

Have you read the Contributing Guidelines?

@gguillemas gguillemas changed the title Default to requiring authentication by default Require authentication by default May 17, 2024
@gguillemas gguillemas added the topic:security This is related to security label May 21, 2024
@gguillemas gguillemas marked this pull request as ready for review May 21, 2024 09:41
@gguillemas gguillemas requested review from a team and tobiemh as code owners May 21, 2024 09:41
@kearfy
Copy link
Member

kearfy commented May 21, 2024

@gguillemas should the flag possibly be --no-auth? I think that would be easier to remember and to type in compared to --unauthenticated

@gguillemas
Copy link
Contributor Author

gguillemas commented May 21, 2024

@gguillemas should the flag possibly be --no-auth? I think that would be easier to remember and to type in compared to --unauthenticated

I was thinking the same thing originally. However, once I got down to implementing the change, I did not like the double negation of --no-auth being false by default, which may be a bit confusing. More importantly, I think not having authentication (i.e. --no-auth) and allowing unauthenticated access (i.e. --unauthenticated) are different things. I was worried some users would assume that --no-auth would prevent anyone from authenticating, when in reality it does quite the opposite and grants complete unauthenticated access (i.e. without credentials) to anyone.

@gguillemas gguillemas enabled auto-merge May 22, 2024 07:47
Copy link
Contributor

@emmanuel-keller emmanuel-keller left a comment

Choose a reason for hiding this comment

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

LGTM!

@gguillemas gguillemas added this pull request to the merge queue May 22, 2024
Merged via the queue into main with commit 246e802 May 22, 2024
22 checks passed
@gguillemas gguillemas deleted the gerard/default-auth branch May 22, 2024 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic:security This is related to security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants