Skip to content

Disable password authentification on empty password#338

Merged
antoniomika merged 1 commit intoantoniomika:mainfrom
carlfriedrich:disable-password-auth-on-empty-password
Mar 4, 2025
Merged

Disable password authentification on empty password#338
antoniomika merged 1 commit intoantoniomika:mainfrom
carlfriedrich:disable-password-auth-on-empty-password

Conversation

@carlfriedrich
Copy link
Copy Markdown
Contributor

@carlfriedrich carlfriedrich commented Mar 4, 2025

If no password is set, disable password authentification completely by setting an empty password callback function. This prevents brute force attacks guessing the password and hence reduces server load and log amount.

@antoniomika
Copy link
Copy Markdown
Owner

Hey @carlfriedrich!

Thanks for your contribution! Could you update your changes to not set the password callback to a function and instead set it to nil if it's an empty string? That get's rid of the branching if logic.

@carlfriedrich carlfriedrich force-pushed the disable-password-auth-on-empty-password branch from 5a962d2 to 7305d58 Compare March 4, 2025 13:37
@carlfriedrich
Copy link
Copy Markdown
Contributor Author

@antoniomika Thanks for the quick reply! Not sure if I got it correctly, but I pushed a new version without the else branch. Is this what you intended?

@carlfriedrich
Copy link
Copy Markdown
Contributor Author

Change looks bigger than it is now, though, due to changed indentation. :-)

@antoniomika
Copy link
Copy Markdown
Owner

@carlfriedrich

Heh, that isn't exactly what I meant. I meant more to revert your changes and add an if statement after the ssh config is defined that sets sshConfig.PasswordCallback to nil.

Should only be the lines of the if clause added (3ish lines) and reads a bit more clean (to me at least).

@carlfriedrich carlfriedrich force-pushed the disable-password-auth-on-empty-password branch from 7305d58 to f50966f Compare March 4, 2025 13:47
@carlfriedrich
Copy link
Copy Markdown
Contributor Author

@antoniomika Ah, got it! 🙈 That's a much cleaner change indeed. Pushed updated version.

@antoniomika
Copy link
Copy Markdown
Owner

@carlfriedrich

Awesome, looks great! Also realized we forgot one check. Can you also make sure the config for "authentication-password-request-url" is empty?

I think that's it!

@carlfriedrich carlfriedrich force-pushed the disable-password-auth-on-empty-password branch from f50966f to e93a30a Compare March 4, 2025 13:54
@carlfriedrich
Copy link
Copy Markdown
Contributor Author

@antoniomika Done.

antoniomika
antoniomika previously approved these changes Mar 4, 2025
Copy link
Copy Markdown
Owner

@antoniomika antoniomika left a comment

Choose a reason for hiding this comment

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

Lgtm 👍

@antoniomika
Copy link
Copy Markdown
Owner

@carlfriedrich hrm the linter doesn't seem to like the changes. I'll add this plus in a lint fix once I'm at my computer. Otherwise, feel free to run go fmt

If no password is set, disable password authentification completely
by setting an empty password callback function. This prevents brute
force attacks guessing the password and hence reduces server load and
log amount.
@carlfriedrich carlfriedrich force-pushed the disable-password-auth-on-empty-password branch from e93a30a to 987d2c6 Compare March 4, 2025 16:25
@carlfriedrich
Copy link
Copy Markdown
Contributor Author

@antoniomika Thank you! I updated the commit, linter is happy now.

Copy link
Copy Markdown
Owner

@antoniomika antoniomika left a comment

Choose a reason for hiding this comment

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

Lgtm 👍

@antoniomika antoniomika merged commit 62f2590 into antoniomika:main Mar 4, 2025
1 of 2 checks passed
@antoniomika
Copy link
Copy Markdown
Owner

Thanks for your contribution @carlfriedrich!

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.

2 participants