oxidized-web icon indicating copy to clipboard operation
oxidized-web copied to clipboard

Add necessary stuff before oxidized base patch to hide enable password from REST API

Open Tibshot opened this issue 9 years ago • 30 comments
trafficstars

I would like to have the ability to hide enable password from oxidized's REST API like:

{ "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": "HIDDEN" }

This patch doesn't break actual setup for users using clear password.

Patch for oxidized base project is coming (TM).

Tib

Tibshot avatar Apr 12 '16 15:04 Tibshot

Looks good to me.

supertylerc avatar Apr 12 '16 15:04 supertylerc

We already have ':remove_secret' var in configs, which models can use to remove secrets from config.

Should we use this, or new? I don't have strong opinion, just pointed it out.

ytti avatar Apr 12 '16 17:04 ytti

We already have ':remove_secret' var in configs, which models can use to remove secrets from config.

I like this idea :smile:

danilopopeye avatar Apr 12 '16 17:04 danilopopeye

From my point of view, I don't understand why :remove_secret is a variable. I prefer the way to define it via rest configuration scope but I agree that is purely subjective...

(Moreover, I didn't know :remove_secret existed before you speak about it :P)

Tibshot avatar Apr 12 '16 19:04 Tibshot

If it would be under rest, do you think it would be still logical that models use it to determine if model will remove secrets from config?

Clearly this is dual-use of single value, I think the use-cases align perfectly, but I might be mistaken.

ytti avatar Apr 12 '16 20:04 ytti

I think that it is more logical if this config param is set under rest scope because it's REST/Web interface that display or not informations.

This directive doesn't remove/rewirte anything within backup files. I have a doubt when you say: "it would be still logical that models use it to determine if model will remove secrets from config".

Does :remove_secret variable remove secret from backuped files ? If yes, my patch don't do that so it's not the same purpose for me. This patch just rewrite the enable var if you have this in your config file:

source:
  default: csv
  csv:
    file: ~/.config/oxidized/router.db
    delimiter: !ruby/regexp /:/
    map:
      name: 0
      model: 1
      username: 2
      password: 3
    vars_map:
      enable: 4

Setting vars_map with enable in config lead /node/show/* API/Web UI calls to return enable password in clear.

I think we don't speak about the same problematic, but I'm not sure ^^. :yum:

Tib

Tibshot avatar Apr 12 '16 21:04 Tibshot

I'm not talking about your proposal. I'm talking about current behaviour of 'remote_secret'.

You expressed confusion why it's 'var', and not under 'rest'. I explained, clearly poorly, that it has nothing to do with the webUI, it's about models removing secret information.

How this relates to your proposal is that I'm wondering do the use-cases of 'remove_secret' and your proposal align perfectly? If they do, we don't need two toggles?

ytti avatar Apr 12 '16 22:04 ytti

Ok get it. :)

When I reviewed the code of remove_secret, I thought these two toggles were OK to run together. Because they don't modify stuff on same level. In my opinion, model.rb don't have to rewrite config stuff (following my exemple: vars_map).

Model.rb removes secrets that are often hashed in backup while REST method add hot removing of enable password in JSON/html response (REST method doesn't open/read/write backup file).

I think 'remove_secret' is very useful because each models have different way to store secrets and this feature is a must have for paranoïd. And I agree that method has nothing to do in WebUI. I didn't want to modify this behaviour, be sure of that.

Finally, I think these methods can clearly work together because their job are not the same at all.

Hoping that I answered your questions :)

Tibshot avatar Apr 13 '16 04:04 Tibshot

Sorry for being thick, I'm not actual sure what your argument is.

Do you think we can 'overload' remove_secret to remove them in web too. Or do you think we should have separate on them?

Ultimately this boils down to 'is there use-case, where someone wants to filter secrets ONLY from configs or ONLY from data'. I don't know answer, I would err towards 'lets use same'.

ytti avatar Apr 13 '16 14:04 ytti

No problem.

Actually, I don't have strict idea on this question. I think it could depends of use case as you said before.

From my business case, I need to keep secret in backuped conf but I need to hide enable from API/HTML response. Tricky. :smiley: In case of restore, I get file directly from Oxidized, without recreating account or role (lazy). During some internals tool use API for monitoring or inventory. We manage url access with access list in web server directly to isolate them.

I get a bug with this PR, I need to continue to work on it. Nodes consider enable password as 'HIDDEN' for all equipments after first check if I request a web page >_<

Don't accept it for the moment, I will commit a fix soon directly in that PR I guess.

Tibshot avatar Apr 14 '16 09:04 Tibshot

Ok. Excellent, your use-case proves we can't overload the configurations and need separate toggles for each.

So we'll go with your original idea.

ytti avatar Apr 14 '16 09:04 ytti

Ok, great. :)

I find a way to debug this PR, so, I'll test it during the night and I'll push new commit tomorrow to this PR if it's OK.

I think that some volunteers will be necessary to test this patch. I haven't a big machine pool at this time to correctly test it.

Tibshot avatar Apr 14 '16 16:04 Tibshot

Yeah I think we generally need lot of thought about testing and quality, I've been slacking on that, and it's really hurting the project. Currently after changing jobs, I don't even run oxidized myself, and have limited access to devices, it's not good for the project at all :/

ytti avatar Apr 14 '16 17:04 ytti

We are very interested in this patch. We are willing to test. Some equipment include: CISCO FTTXs, Aironet APs, Routers(1900s,2900s,6400s,6500,6800s,Nexus7K) HP Procurve switches (several models)

Please let me know if that will work. Also, if i can help, how would i apply said patch?

Thanks Dave

davama avatar Apr 14 '16 17:04 davama

BTW i tried was was suggested here: https://github.com/ytti/oxidized/pull/391

rest_hide_enable: true

It seems that it did hide it from HP configs only.

davama avatar Apr 14 '16 17:04 davama

Hi Dave,

PR 391 from oxidized repo is mine too, it has been developed to work with this current PR. I didn't want manage Oxidized config in web project. :)

Hmm, only HP ? I work with Brocade devices and that works like a charm. Did you test it ?

If you want to test this PR, you will need to use https://github.com/ytti/oxidized/pull/391 too. They work together.

By default, if you don't use https://github.com/ytti/oxidized/pull/391, Oxidized Web will work as usual (no hidden enable).

Hoping I'm not confusing...

Tib

Tibshot avatar Apr 14 '16 18:04 Tibshot

I commited new stuff.

PR contains all necessary things in order to use hide_enable in oxidized-web.

ytti/oxidized#391 needs to be accepted too for configuring rest_hide_enable: true in oxidized config file.

My test devices are OK (2 days and 1 night of tests) and REST/WebUI interfaces answer correctly with 'HIDDEN'.

Tib

Tibshot avatar Apr 15 '16 16:04 Tibshot

Hello Tib Thanks for the help. I was out so i was not able to test sooner.

After a manual attempt of changing all committed files from this PR i just decided, as your note stated from the get go, to clone your oxidized-web fork into our test environment.

Also made sure PR#391 changes were done. (core.rb, config.rb and rest_hide_enable: true)

I too get the an answer of 'HIDDEN'.

Unfortunately i can still see the 'username priv 15 secret 5 blahblah' on all devices.

Btw, i had made a mistake on the procurve comment. password is still there.

I hope i followed the instructions properly. Let me know if there is something i missed.

Dave

davama avatar Apr 18 '16 14:04 davama

Hello Dave,

Thank you for testing my PRs anyway :)

When you say: "Unfortunately i can still see the 'username priv 15 secret 5 blahblah' on all devices.", you speak about backuped configuration, reachable via /node/fetch/your_device_name ? If yes, this is normal. We decided to split response between, for example:

/node/fetch/your_device_name # SECRET ARE NOT HIDDEN

and

/nodes # PLAIN ENABLE PASS HIDDEN IN HTTP RESPONSE /node/show/your_device_name # PLAIN ENABLE PASS HIDDEN IN HTTP RESPONSE

If you want to remove secret hashes from backuped configuration too, you need to set :remove_secret in var like @ytti said earlier in this thread.

You can check code on this file: https://github.com/ytti/oxidized/blob/979e6e7260e66c8fecabd6f424e166ba1193f54c/lib/oxidized/model/model.rb at line 87.

I didn't test remove_secret but setting this in oxidized config can suit for your case.

Hope that can help you,

Tib

PS: If you think I didn't understand what you said previously, coyy/paste response of what is wrong for you (with personnal/business stuffs hidden of course).

Tibshot avatar Apr 18 '16 19:04 Tibshot

That is much clearer, thanks.

If you want to remove secret hashes from backuped configuration too, you need to set :remove_secret in var like @ytti said earlier in this thread.

this is what i did: rest_hide_enable: true #vars: {} vars: {remove_secret}

you speak about backuped configuration, reachable via /node/fetch/your_device_name ?

Yes, that's what i was referring to. The same line appears in the git repo too.

When i read your comment on

PLAIN ENABLE PASS HIDDEN

I think it dawned on me that maybe you were discussing the actual "enable password" line rather than the local username account line. Either way i created an enable password to test but both the 'enable pass' and 'username acc' lines still comes up on the /node/fetch/device_name page, as it was designed (if i understand it correctly). But it also appears in the git repo (which im assuming is the backuped config)

Im assuming that if the backuped config secrets are removed then the /nodes/fetch/device_name page will also be removed. Is that correct? Can both be removed?

Hope it's clear. Thank you for the continued support.

Dave

davama avatar Apr 19 '16 13:04 davama

Yeah, actually, /node/fetch/device_name page is not managed with rest_hide_enable.

But remove_secret in vars normally removes stuff from backuped config showed on /node/fetch/device_name page.

"Im assuming that if the backuped config secrets are removed then the /nodes/fetch/device_name page will also be removed. Is that correct? " -> Yes, /nodes/fetch/device_name page points to your backend storage.

FYI, devices managed with remove_secret are listed here: https://github.com/ytti/oxidized/search?p=1&q=secret

About your configuration, I would have put:

rest_hide_enable: true
vars: 
  remove_secret: true

My equipment aren't 'remove_secret' aware so I can't help you to debug more :(

Tib

Tibshot avatar Apr 19 '16 16:04 Tibshot

Thank you for the link. It was very useful. Sorry but this is going to be fairly long, so please bear with me. (hope it's useful to you or someone else like me)

Yeah, actually, /node/fetch/device_name page is not managed with rest_hide_enable.

So what does rest_hide_enable actually do? Do i really need it?

seeing the devices with remove_secret link, i made some modifications to suit our needs. I preface that im no Ruby expert so suggestions as to how to better do it are very welcomed.

ciscosmb.rb; added the following:

cfg.gsub! /^(username (\S+) password (\S+)) (\S+)./, '\1 /, '\1

As you can see everything stays but just the 'password/key' itself is removed. Output looks as so:

username manager password encrypted

ios.rb; added/edited the following:

#cfg.gsub! /username (\S+) privilege (\d+) (\S+)./, '/, '\1

Output looks as so:

username manager privilege 15 secret 5

procurve.rb; added/edited the following:

#cfg.gsub! /^(radius-server host)./, '\1 ' cfg.gsub! /^(radius-server key)./, '\1

We didn't see a need to hide the actual radius-server hosts, so we commented. Interestingly our output on the password part was in 2 lines. (HP always has to be special) Thats why you see the '\n' in the code.

Output is as so:

password manager user-name "manager" sha1

Noticed that nxos.rb was not yet supported. So i just added the 'secret' block:

cmd :secret do |cfg| cfg.gsub! /^(snmp-server community)./, '\1 ' cfg.gsub! /^(snmp-server user (\S+) (\S+) auth (\S+)) (\S+) (priv) (\S+)/, '\1 ' cfg.gsub! /^(username \S+ password \d) (\S+)/, '\1 /, '\1

output:

username manager password 5

Sorry again for the long post. This is working very good!

Any ETA as to when this PR will be added to the next version of oxidized?

Thanks again, Dave

davama avatar Apr 20 '16 13:04 davama

You're welcome. :)

rest_hide_enable just put 'HIDDEN' in HTTP result if you specified plain enable password in source var_maps like:

# part of oxidized configuration file
source:
  default: csv
  csv:
    file: "/etc/oxidized/router.db"
    delimiter: !ruby/regexp /:/
    map:
      name: 0
      model: 1
      username: 2
      password: 3
    vars_map:
      enable: 4

rest_hide_enable DOESN'T remove/rewrite anything in node informations, it just hide any plain password used to connect to equipments on web interface and json responses. :)

I guess you can need it if you want to hide enable password (if you are using var_maps like my previous yaml example) from users who could have access to oxidized web interface.

Tib

Tibshot avatar Apr 20 '16 14:04 Tibshot

"Any ETA as to when this PR will be added to the next version of oxidized?"

since it's been 5 years since that question, would it be possible to just give a glance and sign this off in favour of progress?

FlorianHeigl avatar May 26 '21 20:05 FlorianHeigl