dmwm / WMCore

Core workflow management components for CMS.
Apache License 2.0
45 stars 106 forks source link

Remove user:pass from spec #2397

Closed cinquo closed 11 years ago

cinquo commented 12 years ago

The user:pass are being written on the spec file and then easily showed through regmgr:

{{{ workflow-name.persistency.specUrl workflow-name.tasks.Analysis.input.acdc.server workflow-name.tasks.Analysis.input.splitting.couchURL workflow-name.request.schema.ACDCServer workflow-name.request.schema.ACDCUrl }}}

cinquo commented 12 years ago

mcinquil: It is not just for the analysis (I noticed from my setup), but also for other workflow:

{{{ workflow-name.tasks.StepOneProc.steps.cmsRun1.application.configuration.retrieveConfigUr }}}

stuartw commented 12 years ago

swakef: Not sure what we can do about this (*), I would suggest the solution is to put these couchdb's behind a frontend and use certificate authorisation.

In fact i think we have had this discussion before, so to reframe it: what couchdb's do we need to hold specs and are there any others not covered by the ones now on cmsweb? And if so can we put them behind a frontend?

(*) I guess we could hide the passwords in the viewer, but then i think any cms user can download the spec and find any embedded passwords.

cinquo commented 12 years ago

mcinquil: Certificated auth means that just the operator (who decided the password) can see these? I think that even hiding doesn't solve the issue: writing password to file showed through http should be avoided.

drsm79 commented 12 years ago

metson: Cert auth would mean you wouldn't need the password in there at all. I'd say that was a requirement.

sfoulkes commented 12 years ago

sfoulkes: Replying to [ticket:2397 mcinquil]:

{{{ workflow-name.persistency.specUrl workflow-name.tasks.Analysis.input.acdc.server workflow-name.tasks.Analysis.input.splitting.couchURL workflow-name.request.schema.ACDCServer workflow-name.request.schema.ACDCUrl }}}

None of these actually need password since we only read from them. The code that is storing these URLs just needs to strip out the passwords. This brings up the question of whether we want to continue to allow anonymous reads of the couchdb machines? Is fire walling them good enough?

drsm79 commented 12 years ago

metson: Evidently not...

cinquo commented 12 years ago

mcinquil: It looks here with secured deploy things are fine:

https://cmsweb-testbed.cern.ch/reqmgr/view/showWorkload?url=https://cmsweb-testbed.cern.ch/couchdb/reqmgr_workload_cache/dataops_Backfill_110919_01_T1_US_FNAL_REDIGITEST_110919_215331/spec

sfoulkes commented 12 years ago

sfoulkes: Dave, review? Patch makes sure we don't leak a password in the spec URL. The version of the ReqMgr on the trunk already strips out passwords.

evansde77 commented 12 years ago

evansde: (In 088bfffe7ac389bba812736dfcecbcc887c96c20) From e9c3e21646dffae04159f7c3824860ddf5b3c4b5 Mon Sep 17 00:00:00 2001 Subject: [PATCH] Remove any passwords that may have snuck into the persistency URL. Fixes #2397.

From: Steve Foulkes sfoulkes@fnal.gov

ghost commented 12 years ago

lat: There are also in the workflow specs.

{{{

!python

FOO.persistency.specUrl FOO.tasks.StepOneProc.steps.cmsRun1.application.configuration.configUrl FOO.tasks.StepOneProc.steps.cmsRun1.application.configuration.retrieveConfigUrl FOO.tasks.StepOneProc.steps.cmsRun1.application.configuration.configCacheUrl FOO.request.schema.CouchURL }}}

ghost commented 12 years ago

lat: Just to be clear, I don't think the patch that was already applied would sanitise those.

ghost commented 12 years ago

lat: And to be even more clear, there are tons of different variations on those variables if you poke around the workflows. I don't know if they all come in some small subset that can be handled in small code location.

sfoulkes commented 12 years ago

sfoulkes: Replying to [comment:11 lat]:

There are also in the workflow specs.

{{{

!python

FOO.persistency.specUrl FOO.tasks.StepOneProc.steps.cmsRun1.application.configuration.configUrl FOO.tasks.StepOneProc.steps.cmsRun1.application.configuration.retrieveConfigUrl FOO.tasks.StepOneProc.steps.cmsRun1.application.configuration.configCacheUrl FOO.request.schema.CouchURL }}}

The patch that was committed will fix FOO.persistency.specUrl. The others are fixed in newer versions of the ReqMgr. Code in question: https://svnweb.cern.ch/trac/CMSDMWM/browser/WMCore/trunk/src/python/WMCore/HTTPFrontEnd/RequestManager/WebRequestSchema.py#L93

The CouchURL that is passed in via the schema is what used to build all of the ConfigCache URLs.

ticoann commented 12 years ago

sryu: I think the problem is partially in wmbs-mod-config. I added the patch which I know of user and pass for couch is not needed. But not sure for other cases. Maybe be we should have some config name convention to distinguish. Patch is updated but Is not for commit just for examples.

sfoulkes commented 12 years ago

sfoulkes: Patch attached fixes the problems with showWorkload.

ghost commented 12 years ago

lat: Following up discussion from #2416 here.

Thanks for moving reqmgr to local interface; I confirm it's no longer accessible. The couch database is still accessible and still contains URLs with passwords (for example http://vocms144.cern.ch:5984/reqmgrdb/cmsdataops_05Aug2011ReRecoMuHad_110805_145057 or http://vocms144.cern.ch:5984/_utils/document.html?reqmgrdb/cmsdataops_05Aug2011ReRecoBTag_110805_145058). It's pretty to navigate to them via _all_docs at the top level, or especially via the _utils.

So definitely much progress, but what's going to happen to the data? Are there other places at CERN or FNAL with such user/password URLs? Fortunately this particular password has been changed, and in searches I made, it didn't come up - so that's good.

Can't comment much on the patch, sorry.

sfoulkes commented 12 years ago

sfoulkes: Replying to [comment:17 lat]:

So definitely much progress, but what's going to happen to the data? Are there other places at CERN or FNAL with such user/password URLs? Fortunately this particular password has been changed, and in searches I made, it didn't come up - so that's good.

There aren't any other places that will have user/password URLs. The only place they would be stored would be in the spec, which is what you posted above. The ReqMgr now strips out any username and password before the URLs are passed to the spec creation code so this won't happen again.

evansde77 commented 12 years ago

evansde: Steves patch approved and committed...

ticoann commented 12 years ago

sryu: Steve, I added some partial patch. But we have to check all the couch url in the config whether user/passwd is needed. I just don't know much of other components.

ghost commented 12 years ago

lat: Replying to [comment:18 sfoulkes]:

There aren't any other places that will have user/password URLs.

Mmm. vocms104 has the same problem. Using the user name and password I found in there, I was able to connect to CouchDB, gain myself admin rights, create new admin users, create and delete databases, and modify existing documents.

I was also able to create a new (non-admin) user on vocms144, using which I could modify existing documents, create new ones and delete existing documents and fields.

Me thinks we have a problem, and need a solution, pronto. Preferably shut down for weekend until we can come up with a solution. I thought these instances were supposed to be locally firewalled, and only push information out - what's up with having the CMS production framework running with open firewall and next to no authentication at all!?

ghost commented 12 years ago

lat: I was also able to connect to http://cmssrv98.fnal.gov:5984/_utils, create an account and databases, and modify existing documents.

ghost commented 12 years ago

lat: Correction, couldn't create a database at cmssrv98, but did manage to modify, create and delete documents, and create an account for myself.

ghost commented 12 years ago

lat: It also looks like I can modify database contents via https://cmsweb.cern.ch/couchdb/_utils/ in reqmgr. Diego tells me it's missing validate_doc_update in design document. In other parts, like stage manager databases, rogue updates are in fact properly blocked (and there's a validate_doc_update in design doc). In CMSWEB installs user management actually works properly, but Diego says it's only possible if there's front-end in front of couch.

ghost commented 12 years ago

lat: I found another couch database on cmssrv98, at a different port, with usual capabilities. I was also able to login into cmst1 user at cmssrv112.fnal.gov using the password from the vocms144 instance URLs. I decided not to try creating more havoc around couch databases around the world. I need to do something else now.

DMWMBot commented 12 years ago

mmascher: There is an error in the patch 0001-Use-only-the-request-name-to-retrieve-requests-not-t.patch at line 231:

{{{ except Exception, RuntimeError ex: }}}

spigad commented 12 years ago

spiga: Replying to [comment:21 lat]:

Replying to [comment:18 sfoulkes]:

There aren't any other places that will have user/password URLs.

Mmm. vocms104 has the same problem.

Claudio who is running that server is now fully aware of the issue. He is going to restart the server applying the provided patches an FWIK he also changed all the passwd.

Using the user name and password I found in there, I was able to connect to CouchDB, gain myself admin rights, create new admin users, create and delete databases, and modify existing documents.

I was also able to create a new (non-admin) user on vocms144, using which I could modify existing documents, create new ones and delete existing documents and fields.

Me thinks we have a problem, and need a solution, pronto. Preferably shut down for weekend until we can come up with a solution. I thought these instances were supposed to be locally firewalled, and only push information out - what's up with having the CMS production framework running with open firewall and next to no authentication at all!?

DMWMBot commented 12 years ago

mmascher: Claudio noted another issue probably related to the mod-config patch. The config file generetad contained:

{{{ config.WorkQueueManager.queueParams = {'ParentQueueCouchUrl': {'url': 'http://vocms104.cern.ch:5984/workqueue', 'username': 'integration', 'password': '*****'}} }}}

but while loading it he got:

{{{ RuntimeError: Complex Value type in sequence:<type 'dict'> }}}

Nested dict are not allowed in our ConfigSections

stuartw commented 12 years ago

swakef: patch-series-sryu-don-commit.patch is wrong. The sanitizeURL(args["couch_url"]) line should be sanitizeURL(args["couch_url"])['url']. Also, we can't sanitize the parent couch url as it needs write access to acquire work.

DMWMBot commented 12 years ago

grandi: To be precise the config file generated contained: config.WorkQueueManager.queueParams = {'ParentQueueCouchUrl': {'url': 'http://vocms104.cern.ch:5984/workqueue', 'username': None, 'password': None}}

I tried leaving it as it was and with config.WorkQueueManager.queueParams = {'ParentQueueCouchUrl': {'url': 'http://vocms104.cern.ch:5984/workqueue', 'username': 'integration', 'password': '*****'}}

ticoann commented 12 years ago

sryu: Replying to [comment:29 swakef]:

patch-series-sryu-don-commit.patch is wrong. The sanitizeURL(args["couch_url"]) line should be sanitizeURL(args["couch_url"])['url']. Sorry about that but that patch is just Steve to check whether there might be problem (Steve mentioned the problem as well yesterday). That is why I said don't commit. Anyway, I updated patch, but still this shouldn't be committed. (There are many other places I wasn't sure whether it is sanitized or not.

Also, we can't sanitize the parent couch url as it needs write access to acquire work. I thought for 'acquire work' we don't need user/pass. Isn't that just get call. The only place need them probably for replication. But I thought that authentication and authorization occurs with cert.

ticoann commented 12 years ago

sryu: I think probably we should change to use cert/key to authenticate and authorize couchDB functions as well as control the access of certain futon interface I don't think couchDB supports that, though. I am not suer whether adding apatch front end makes. or is there some lightweight proxy server we can use for that purpose.

stuartw commented 12 years ago

swakef: (In 542061a3021111909fb0445c4842213ed7521572) Sanitize monitoring url's in local workqueue monitoring

See #2397

From: Seangchan Ryu sryu@fnal.gov Signed-off-by: Stuart Wakefield stuart.wakefield@imperial.ac.uk

stuartw commented 12 years ago

swakef: A stab at adding couchdb authentication functions attached. This basically copies the WorkQueue one, with the difference that i've attempted to spot the couchdb's that don't require ReqMgr to have write access and removed it's role ("facops", "web-service"). People who know the couchapps better should have a look.

stuartw commented 12 years ago

swakef: ps. This makes no distinction between creating/updating and deleting a document which may be a restriction appropriate to some couchdb's if so see the stagemanager usage for an example.

PerilousApricot commented 12 years ago

meloam: The patch associated with this ticket has a syntax error. Solid.

{{{

Andrew-Melos-MacBook-Pro:GITHUB-WMCORE meloam$ python2.6 src/python/WMCore/HTTPFrontEnd/RequestManager/ReqMgrBrowser.py File "src/python/WMCore/HTTPFrontEnd/RequestManager/ReqMgrBrowser.py", line 231 except Exception, RuntimeError ex: ^ SyntaxError: invalid syntax

}}}

Try the attached patch.