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

Configure SSL certificate #26

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Configure SSL certificate #26

wants to merge 6 commits into from

Conversation

noxonad
Copy link

@noxonad noxonad commented Dec 29, 2024

Add option for the user to configure SSL certificate and either set one that's self-signed or ignore it completely when creating the request. Might be useful for people self-hosting inside an internal network that can't issue a certificate for validity and have one that of their own that they trust.

Edit: No idea why but all my work got PRd at once.

Changes made also include fix of #23, the config file contains a private information thus should be unreadable for others, and a typo fix in the config file's comment.

The user now can set whether to use an SSL certificate or not, or even
set their own self-signed certificate from the config file. The config
file comes with additional instruction and warning about SSL
certificates.

If ssl_verify isn't set in the config file, it defaults to true.
User config file contains a secret API that shouldn't be readable to
others
Copy link
Owner

@almarklein almarklein left a comment

Choose a reason for hiding this comment

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

Nice, I have one suggestion. Other than that this looks good.

Comment on lines 92 to 93
# Config file contains user secret so it should not be readable to others
os.chmod(filename, 0o640)
Copy link
Owner

Choose a reason for hiding this comment

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

Would be cleaner to do this where the file is created, in prepare_config_file()

Copy link
Author

Choose a reason for hiding this comment

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

What should happen if the config file already exists and the user wants to setup the config again? Should it overwrite the permissions just in case the user have messed with them or leave the permissions unchanged and potentially exposing the user's secret?

Copy link
Owner

Choose a reason for hiding this comment

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

I guess the file can be kindof considered an "implementation detail", so let's set it every time. What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

I think it would be better to check if the existing file is a config file and try to mess less with user files in case it just so happens the user have some important, non-program related information.

I can commit now with that fix, but in the future it would be better to add some additional check, including what if the path is actually a directory and not a file.

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