Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#3038 closed problem (fixed)

data builder requires access to user's home directory

Reported by: dconnolly Owned by: mhoag
Priority: major Milestone: heron-webster-update
Component: data-repository Keywords: security, gpc, public-web
Cc: mhoag, nhgraham, bhamlin, badagarla Blocked By:
Blocking: 2352 Sensitive: no

Description

The data builder writes to ~USER/heron, which conflicts with recent IR VMs and guidelines.

Design options:

  • have the builder write to something like /var/lib/builder/USER/ and symlink ~USER/heron to that.
  • use a small setuid program to move the file from where the builder puts it to where the user will access it

Change History (27)

comment:3 Changed 4 years ago by dconnolly

  • Owner set to dconnolly
  • Status changed from new to assigned

I tried this in ticket:3068#comment:2, but it didn't quite work.

comment:4 Changed 4 years ago by dconnolly

  • Keywords gpc added
  • Milestone changed from heron-verdigris-update to heron-kanopolis-update
  • Priority changed from minor to major

This came up in GPC discussions too; promoting to major for consideration for milestone:heron-kanopolis-update.

comment:5 Changed 4 years ago by dconnolly

  • Milestone changed from heron-kanopolis-update to dconnolly

Milestone renamed

comment:6 Changed 4 years ago by dconnolly

  • Milestone changed from dconnolly to heron-kanopolis-update

Milestone renamed

comment:7 Changed 3 years ago by dconnolly

  • Status changed from assigned to accepted

This is a pain for GPC folks; I meant to work on it today, but...

comment:8 Changed 3 years ago by dconnolly

  • Milestone changed from heron-kanopolis-update to dconnolly

Milestone renamed

comment:9 Changed 3 years ago by dconnolly

  • Milestone changed from dconnolly to heron-kanopolis-update

Milestone renamed

comment:10 Changed 3 years ago by dconnolly

  • Keywords heron-weekly added
  • Milestone changed from heron-kanopolis-update to heron-webster-update

Tagging heron-weekly to request "Discuss remaining major open tickets" from HeronLoad#milestone-close.

comment:11 Changed 3 years ago by dconnolly

  • Keywords heron-weekly removed
  • Owner changed from dconnolly to mhoag
  • Status changed from accepted to assigned

Matt,

I coded up the design I had in mind today [fc25bbe38a07/heron_extract]. I'd like to go over with you:

  • changes since what's deployed in our shop [73f7a0725498/heron_extract]
  • changes since what's released to GPC:BuilderSaga [1b8a53f78be0/heron_extract]
    • I updated the i2b2_vm_deploy branch to no longer provision a unix account, but
    • We haven't been testing the i2b2 vm deployment, so I'm not sure what impact our other changes will have.
    • I want to think about the artifacts thingy in the GPC context.

comment:12 follow-up: Changed 3 years ago by mhoag

  • Owner changed from mhoag to dconnolly

The changes I reviewed look good. (Assign this back to me if you think I should review all the changes since we last deployed, or we can go over them together)

One thing we need to remember after deployment is to configure builder-extract-config (/view/DUA-NG/job/builder-extract-config/) to use the new config parameters (and remove the old one).

Code Review notes

eefafcf52c9d/heron_extract

  • No Comments

fc25bbe38a07/heron_extract

  • dfbuilder.py
    • 108:113 - What necessitated the change from all([...]) to [... is expected not in msg]? The former seems more clear and readable to me (i.e. all of excepted strings are in the msg).
  • dfbuild-extract.conf.example
    • 40:41 - Is it really okay to have a newline in a config assignment? I am surprised that that would work

754f6e932716/heron_extract

  • No Comments

comment:13 in reply to: ↑ 12 ; follow-up: Changed 3 years ago by dconnolly

Replying to mhoag:

fc25bbe38a07/heron_extract

  • dfbuilder.py
    • 108:113 - What necessitated the change from all([...]) to [... is expected not in msg]? The former seems more clear and readable to me (i.e. all of excepted strings are in the msg).

"when email notification test fails, show which part is missing" is the relevant part of the commit message.

That is: when the all() form failed, all I got was "False". In the revised form, the test failure shows the offending item.

  • dfbuild-extract.conf.example
    • 40:41 - Is it really okay to have a newline in a config assignment? I am surprised that that would work

It works if the continuation line starts with a space, I think. Let's see... it seems to be explicitly documented only in the python3 version of the docs:

Values can also span multiple lines, as long as they are indented deeper than the first line of the value.

comment:14 in reply to: ↑ 13 Changed 3 years ago by mhoag

Replying to dconnolly:

"when email notification test fails, show which part is missing" is the relevant part of the commit message.

That is: when the all() form failed, all I got was "False". In the revised form, the test failure shows the offending item.

Makes sense

It works if the continuation line starts with a space, I think. Let's see... it seems to be explicitly documented only in the python3 version of the docs:

Learn something new every day!

comment:15 Changed 3 years ago by dconnolly

  • Owner changed from dconnolly to mhoag

Matt, I think you offered to merge this, though we need to think about unix permissions at deployment time.

comment:16 Changed 3 years ago by ngraham

heron-weekly - keep in Webster.

comment:17 Changed 3 years ago by mhoag

  • Owner changed from mhoag to dconnolly

I never got around to merging this, but it looks like it was merged into date_limits_2352 on 5b6928444031/heron_extract, so I am giving this back to Dan since this will probably go into default when the work on #2352 is merged to default.

comment:18 Changed 3 years ago by dconnolly

  • Blocking set to 2352

comment:19 Changed 3 years ago by mhoag

  • Blocking changed from 2352 to 2352, 3222

comment:20 Changed 3 years ago by ngraham

  • Blocking changed from 2352, 3222 to 2352

comment:21 Changed 3 years ago by dconnolly

I went with the small setuid program; I couldn't work out the permission constraints for the symlink design.

fixed in d09ffba1f5d4/heron_extract along with

  • 4b77938ad955/bmi_ops for the setuid code and
  • various changes to configuration of various jenkins jobs

Summary of the home_too_much_3038 branch:

  1. don't mask bugs in formatting mail messages while ignoring IOErrors [eefafcf52c9d/heron_extract]
  2. no need for privilege to write to home directories [fc25bbe38a07/heron_extract]
  3. use CLI to determine output location rather than config, username, jobname [85629222cdb9/heron_extract]
  4. Merge with default to check unit tests unrelated to home_too_much_3038. [5583fc55e895/heron_extract]
  5. oops... mixed up rd and ed in mk_access [c4e3e84914dc/heron_extract]
  6. Merge with mail-on-fail-3038 [567caf1466f6/heron_extract]
  7. Since output path is now a CLI arg, never mind writing artifacts file [d42dea019e46/heron_extract]
  8. Merge with perf_2365 [ad9f3807a43b/heron_extract]

While I was at it, on mail-on-fail-3038, I changed email notification to happen whether we win or lose:

  1. factor mockConfig() out of Emailer doctest [c2791228f742/heron_extract]
  2. toward context handler for win/lose notification: separate fill_template() from send_message() [6270600d3077/heron_extract]
  3. Mailer.notification() contexthandler notifies on success or failure [24ca0c1df1cd/heron_extract]
  4. Send email notification even if BuilderApp?.run() raises an exception [e52b432555e1/heron_extract]

For reference: hg log --branch home_too_much_3038 --template ' {rev}. {desc|firstline} [{node|short}]\n'|sort -n

comment:22 Changed 3 years ago by dconnolly

  • Resolution set to fixed
  • Status changed from assigned to closed

wierd; I'm pretty sure I closed this already.

comment:23 Changed 3 years ago by mhoag

  • Resolution fixed deleted
  • Status changed from closed to reopened

Uh oh. This doesn't work with our current methodology for dua2redcap-NG 37 (/view/DUA-NG/job/dua2redcap-NG/37/console) since we write some views to the db in that job in preparation for making the csvs.

I ran the idea by Nathan that we copy the db for Jenkins's usage create the artifacts and then dispose of it. He seemed to think that was a good idea. Going to modify dua2redcap-NG (/view/DUA-NG/job/dua2redcap-NG/) as such.

comment:24 Changed 3 years ago by mhoag

  • Owner changed from dconnolly to mhoag
  • Status changed from reopened to assigned

comment:25 follow-up: Changed 3 years ago by mhoag

changes to get this to work in this diff (/view/DUA-NG/job/dua2redcap-NG/jobConfigHistory/showDiffFiles?timestamp1=2015-02-18_09-03-30&timestamp2=2015-02-27_13-24-18)

But ran into another hiccup. dua2redcap_upload (/view/DUA-NG/job/dua2redcap_upload/) relies on a view (redcap_data) being created in the original db which since it is read only to Jenkins, never gets created.

To get around this I:

  1. Copied the zip artifact from dua2redcap-NG
  2. Unzipped from contents
  3. remade the sqlite3 db from *.sql.gz
  4. ran dua2redcap_upload (/view/DUA-NG/job/dua2redcap_upload/) using the remade db

diff of changes (/view/DUA-NG/job/dua2redcap_upload/jobConfigHistory/showDiffFiles?timestamp1=2015-02-18_10-31-22&timestamp2=2015-02-27_13-44-58)

Yeah redcap data started uploading... build 38 (/view/DUA-NG/job/dua2redcap_upload/38/console) but it still failed because of redcap validation. Boo. Technically the work here is done, but we still need to track this redcap validation error somewhere...

comment:26 Changed 3 years ago by mhoag

Also there was a problem early on with mail-on-fail side effort of this ticket. Effectively the email context manager was eating all of the errors when the builder_extract fell over (i.e. the user was getting an e-mail that there was a problem with their data set, but the builds logs indicated no such problem. I made sure that the context manager propagated the error after it sent the email for failure (8a25738ed120/heron_extract, 4df6dcae66f0/heron_extract). Merged to default on d8f3f0607896/heron_extract.

comment:27 in reply to: ↑ 25 Changed 3 years ago by mhoag

  • Resolution set to fixed
  • Status changed from assigned to closed

Replying to mhoag:

Yeah redcap data started uploading... build 38 (/view/DUA-NG/job/dua2redcap_upload/38/console) but it still failed because of redcap validation. Boo. Technically the work here is done, but we still need to track this redcap validation error somewhere...

I was able to get dua2redcap_upload (/view/DUA-NG/job/dua2redcap_upload/) to work successfully on a build 40 (/view/DUA-NG/job/dua2redcap_upload/40/) where the extract had no modifiers.

The errors are looking similar to the ones seen in ticket:2813#comment:17. Going to reopen that ticket and close this one.

Only reason I can think of to potentially re-open this ticket is if we decided that we should use the first design option noted in the description:

  • have the builder write to something like /var/lib/builder/USER/ and symlink ~USER/heron to that.

instead of the one implemented.

comment:28 Changed 3 years ago by ngraham

  • Keywords public-web added

comment:29 Changed 3 years ago by mprittie

  • Sensitive unset
Note: See TracTickets for help on using tickets.