-
Notifications
You must be signed in to change notification settings - Fork 1k
Add new config param: 'rest_hide_enable', to hide enable password from REST API #391
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
Conversation
lib/oxidized/config.rb
Outdated
| asetus.default.retries = 3 | ||
| asetus.default.prompt = /^([\w.@-]+[#>]\s?)$/ | ||
| asetus.default.rest = '127.0.0.1:8888' # or false to disable | ||
| asetus.default.rest_hide_enable = false # to don't break actual setup, we set to false |
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.
to so don't break actual current setup
but it's only 💅 😃
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.
Well... yes ! 😋
|
I'm so tempted to change the config to But that would break backwards... Is 'enable' reasonable word? Are we only hiding enable, if so, why? Why not hide passwords etc potentially discreet stuff? |
|
I can modify this way, seems to be more logical to me, but I would do this way: rest.url = x
rest.hide_enable = y'enable' is better for me because it's mapped in vars_map like: vars_map:
enable: 4Setting it with 'secrets' can lead to poor understanding. For 'rest.url', I totally agree with you. Tib |
|
On 16 April 2016 at 20:47, Tib [email protected] wrote:
I'm not sure about it, because if we do it, we'll break backwards :/. vars_map: enable is just one thing, it might be another thing for another ++ytti |
|
Yes, agree for backward compatibility. Maybe, can we hard modify this in future releases ? But at this time, I can propose to stay with actual rest and modify rest_hide_enable like rest_hide_vars: rest = 'an url'
rest_hide_vars = ['enable', 'another_field' , 'etc' ] # this can be set on empty by defaultDo you want to hide var_map only or all node's variables ? (Like update node date or status) Have you examples on another var_map value ? With my uses, I just have 'enable' to hide. :/ We can use this without break current production installations. But I need to correct my oxidized-web PR too. :) |
|
On 17 April 2016 at 11:26, Tib [email protected] wrote:
ACK. I think we again have to go with your original plan and break this
Yeah, we can't break this backwards, I'd like to, but we can't. You're ++ytti |
|
Definitely yes, it would be great to hide another things. I will commit (maybe today or tomorrow) new stuff in order to hide whatever we want from WebUI/REST. As example, before, we got this from oxidized-web: {
"full_name": "a-device",
"group": "default",
"ip": "10.10.10.10",
"last": {
"end": "2016-04-12 12:21:51 UTC",
"start": "2016-04-12 12:21:25 UTC",
"status": "success",
"time": 26.277578618
},
"model": "a-model",
"name": "a-device",
"status": "success",
"time": "2016-04-12 12:21:51 UTC",
"vars": {
"enable": "clear_password_here"
}}After, if we configure in config file (it's for example purpose, we just don't give a shit about status and ip here, it could be other stuff in real life). This yaml fromatting can change, depends of ability to do it proprely, but, at this time, I find it well: rest_hide_vars:
- ip
- status
- vars: enableIn oxidized/config.rb, we will modify file like this: asetus.default.rest_hide_vars = [] # empty by default, backward compatibilityAnd we will get from oxidized-web: {
"full_name": "a-device",
"group": "default",
"ip": 'HIDDEN',
"last": {
"end": "2016-04-12 12:21:51 UTC",
"start": "2016-04-12 12:21:25 UTC",
"status": 'HIDDEN',
"time": 26.277578618
},
"model": "a-model",
"name": "a-device",
"status": "success",
"time": "2016-04-12 12:21:51 UTC",
"vars": {
"enable": 'HIDDEN'
}}This next push WON'T BREAK backward. By default, this just won't be configured. Are you OK with this way ? As we say this week-end, we can start to think about new config for next releases but these won't be in this commit. Maybe in an new issue to don't loss this thread. |
|
This is beginning to sound like the webUI needs logins/views and all these need to have attribute 'hidden' which can be true or false. I was thinking that we'd just have one keyword for web, which would hide all potentially discreet stuff. I don't think IP is one. |
|
Hi, Hiding IP address was an example to show you what my proposal could do if I code it :). Your way of thinking about hide potential discret stuff doesn't feet because, discret stuff are subjectives and can change between companies and use case of oxidized. Some companies can hide IP and password, others can hide status and enable, etc, etc. It's not to Oxidized people or project to chose what to hide, imho. In my previous comment way, I demonstrate that company/admin can chose what they think rightful to show and my proposal could manage this way. Tib |
|
If we really need this for arbitrary thing, then I think current approach may not be optimal. Then I'm thinking this should be 100% in oxidized-web. Some UI editor where you can toggle 'hidden' attribute of each field, if set to true, it's not rendered. Potentially UI view per user. Remember there is still API, which will puke shit, so if users have access to API as well as the webUI, they will still get secrets. |
|
If you think, theses stuff has to be in oxidized-web, we need DB backend to store configuration values. |
|
ACK. |
|
Hi, We need to list all feature we want in order to code them properly. Moreover, I think that for the moment, my PRs can do the job (just hide plain enable password). But for future releases, indeed, we have to manage this from oxidized-web. And it could be very interesting for users. Unfortunately, I cannot code these features in oxidized-web at the moment, but I stay available to speak about how to design this. Tib |
|
I'd like to try and clean up some old pull requests if we can. @PANZERBARON is this something you could consider adding to oxidized web and can we close this down? |
Add configuration parameter 'rest_hide_enable' like:
I didn't update README to don't disrupt users.
This param is set at false to don't break setup using clear password.
This commit go with this PR: ytti/oxidized-web#99
BE CAREFUL: the third argument in core.rb on line 27 needs to update oxidized-web. It needs strict version requirement in gemspec. Using oxidized-web without linked PR breaks oxidized.
Tib