-
Notifications
You must be signed in to change notification settings - Fork 51
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
Update configuration of python-cli to configuration policy #672
Update configuration of python-cli to configuration policy #672
Conversation
DEFAULT_SERVER_ADDR = os.environ.get("SERVER_ADDR", "grpc://127.0.0.1:55555") | ||
DEFAULT_LOGGING_CONFIG = os.environ.get("LOGGING_CONFIG", os.path.join(scriptDir, 'logging.ini')) | ||
DEFAULT_TOKEN_OR_TOKENFILE = os.environ.get("TOKEN_OR_TOKENFILE", None) | ||
DEFAULT_CERTIFICATE = os.environ.get("CERTIFICATE", None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is our view on mutual authentication? AFAIK we do not currently support it, as broker does not check/request client cert. I would prefer if we either spend some effort to support it, or conclude that it likely never will be supported and remove all client certs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would vote for removing all that. I don't think they even enable additional security goals over authorisation tokens, which also need to be signed by a trusted authority.
The only advantage is, that TLS cleint auth kicks in "earlier" in the stack on transport level, but even though: We did not have it implemented and it is not on the roadmap, so let's rather clean up here. IMO not much we loose if at a later point we DO decide to implement this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the conclusion of client certificates (for mutual auth). Remove as part of this PR?
If so we need to decide how "deep" we want to remove them as part of this PR. Only in client, but also the client key/cert itself and the script generating it?
I think remove them, but potentially not necessarily in this PR. @erikbosch as you have reviewed this, you have a better overview, if it is "trivial" or we rather do it separately (I think we would also need to scour documentation, and remove informaton there) |
Lets do it separately later. Could possibly require some extra work here and there to remove all traces of it. |
@lukasmittag @SebastianSchildt - Is this a merge candidate to include in v0.4.1? |
I think so |
@lukasmittag - do you want to rebase and fix conflict? So it can be included in 0.4.1 |
will do |
0639cfb
to
fc94ca4
Compare
To meet the discussed configuration policy here #589 this adds in a first step some environment variables for the cli arguments. Futhermore this should be a place to discuss the removal of default tokens. The default tokens were not used anymore because they got overwritten. This gets fixed with this PR.
The question is do we want to keep default tokens as default usage in or do we just want to keep them in the container that they can manually be used.
@argerus @erikbosch @SebastianSchildt @rafaeling