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

Disallow dangerous parameters in the config file #217

Open
mariocynicys opened this issue May 7, 2023 · 6 comments · May be fixed by #218
Open

Disallow dangerous parameters in the config file #217

mariocynicys opened this issue May 7, 2023 · 6 comments · May be fixed by #218
Labels
good first issue Good for newcomers

Comments

@mariocynicys
Copy link
Collaborator

Right now, the parameters overwrite_key (overwrites* the tower's key and gives it a new identity) and force_update (introduced in #216) are allowed to be set in the configuration file.

Since they are more of a fail-safe and rather dangerous commands, we should assert that the user doesn't set them in the config file and only settable as command line flags.

@mariocynicys mariocynicys added the good first issue Good for newcomers label May 7, 2023
@sr-gi
Copy link
Member

sr-gi commented May 7, 2023

Concept ACK

@anipaul2
Copy link
Contributor

anipaul2 commented May 11, 2023

Right now, the parameters overwrite_key (overwrites* the tower's key and gives it a new identity) and force_update (introduced in #216) are allowed to be set in the configuration file.

Since they are more of a fail-safe and rather dangerous commands, we should assert that the user doesn't set them in the config file and only settable as command line flags.

So, removing the parameters from the config file and allowing the user to use it only in cli?

@mariocynicys mariocynicys changed the title Disallow dangrous parameters in the config file Disallow dangerous parameters in the config file May 11, 2023
@mariocynicys
Copy link
Collaborator Author

So, removing the command from the config file and allowing the user to use it only in cli?

Yeah something like that. You might only disallow it by sanity checking that it's not set. Not that we have to remove it from the Config struct.

@anipaul2
Copy link
Contributor

So, removing the command from the config file and allowing the user to use it only in cli?

Yeah something like that. You might only disallow it by sanity checking that it's not set. Not that we have to remove it from the Config struct.

Got it. I will raise a pr when it's done.

@mariocynicys
Copy link
Collaborator Author

Just noticed there is actually nothing dangerous here because Config::patch_with_options completely ignores the config file value of overwrite_key & force_update and uses the command line options instead (which default to false).

I think we can just note that in a comment in the template config file so that users don't expect something when setting these ignored parameters to true.

Also, write a comment about it in

self.overwrite_key = options.overwrite_key;
self.force_update = options.force_update;
since we all missed it xD.

@sr-gi
Copy link
Member

sr-gi commented Jul 17, 2023

Oh damn 😮!

I still think it may be worth reporting this to the user though instead of just commenting this on the config file. In general, if something is not supported, I think the user should be made aware of it explicitly.

Even though I never finished it, a rework of the config to acknowledge where things come from may be good in the long run.

master...sr-gi:rust-teos:config-rework

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants