dump temporary security contexts to file in specified shared temporary directory for use in worker submit scripts by riedel · Pull Request #524 · dask/dask-jobqueue

riedel · 2021-10-20T19:48:29Z

Fixes #520
Closes #534

riedel · 2021-10-20T21:11:35Z

@guillaumeeb: due to your concerns, I extracted this one from #519 to look at it separately. I guess the biggest question is how to best create temporary files.

One maybe better way may be to create a whole directory together with the job script, which also needs to live somewhere.

One could also try to somehow inject this in the scheduler scripts. E.g. if a bash wrapper is used one could use process substitution . But this seems to be a bit complicated to me.

I stuck with this one as it is rather minimal.

guillaumeeb · 2021-10-25T09:15:45Z

@guillaumeeb: due to your concerns, I extracted this one from #519 to look at it separately. I guess the biggest question is how to best create temporary files.

That's a good idea! And I agree with the main concern.

I'll add a review.

guillaumeeb

Thanks again for the work.

@jacobtomlinson we could really use your eyes on this one. Does this file generation looks good to you, and security compliant?

in #496 you said:

I think only supporting file certificates in dask-jobqueue would be reasonable. In places like dask-cloudprovider we are limited to passing the certs in the Dask config because we can't guarantee there will be a shared filesystem. But for HPC I think most cases there will be one.

Did you mean by generating a temporary file? And do you think that in-memory context would be feasible for dask-jobqueue by serialization?

there is also some more information in #520.

riedel · 2021-10-30T08:03:55Z

Forget about the deprecation: accidentally looked at the wrong file. Thought tmpfile was finally deprecated as I remembered that there was a discussion in dask#7800 that it was not needed anymore due to the new python tmpfile, which I misunderstood

So I guess it makes sense then also to use the built-in tmpfile in this PR. I cannot remember why I didn't do that.

riedel · 2021-10-31T16:39:11Z

I think #532 should be completed/merged first. Without any testing this is far to shaky...

guillaumeeb · 2021-11-21T21:26:06Z

So instead of #532, I should look into #533 if I understand?

riedel · 2021-11-27T18:41:31Z

Moved test across all clusters here: #537 (will fail for current implementation)

riedel · 2021-11-28T10:02:28Z

Now, it basically works, the only problem is identifying a writable shared directory for testing. Local still works, htcondor and slurm xfail due to the missing shared filesystem, sge works, however, pbs seems has the permissions on the test directory set as non-writable: what would a better place to write the temporary files?

Second question: Does it make sense to default to the home dir as a shared directory? I guess most clusters work like this, however e.g. the CI does not...

riedel · 2021-11-28T10:07:57Z

BTW: the last remaining error shows that the permissions on the temporary certificate files are set in a secure manner to 0600 .

…i user * added context handler to Job Cluster in core.py * make source dir owned by pbsuser * dynamically select port to fix sporadic port already in use errors

riedel · 2021-11-28T18:09:00Z

I thought I was done: now I get an error in the unmodified
FAILED dask_jobqueue/tests/test_jobqueue_core.py::test_security[LocalCluster]
and
"distributed.scheduler - ERROR - Workers don't have promised key: ['tls://10.1.0.206:44087'], lambda-a54fa6a78ad624099ae9b275efae5945"
this is not good! I was careful not to modify this code...

and I cannot reproduce the error locally. It seems however, the error is related to previously running tests that did not clean up correctly (https://github.com/riedel/dask-jobqueue/runs/4346772070)

riedel

added a few explainations.

guillaumeeb

We need to find an agreement on the configuration name and the default (even that is looks every one agrees for defaulting the config option to None/null).

…ption handling

because it xfails anyways and the problem with the ci is unrelated

tests_required is deprecated but kept as in distributed

guillaumeeb

Just one last suggestion, and then I think we can merge!!

Co-authored-by: Guillaume Eynard-Bontemps <g.eynard.bontemps@gmail.com>

…ask-jobqueue into in_memory_security_contexts_520

riedel · 2021-12-04T11:02:16Z

Since #527 (which is the final thing I want to get merged) passes I am happy now.

guillaumeeb · 2021-12-04T11:20:24Z

Thanks a lot for all the work here @riedel, I know this was long and with a lot of waiting periods.

riedel · 2021-12-04T14:15:44Z

Thank you alot, @guillaumeeb! I am just happy that this part is in now and encrypting traffic now easily works. Also @andersy005 for reviewing.

riedel mentioned this pull request Oct 20, 2021

make tls default if security is set... #519

Closed

riedel changed the title ~~dump temporary security contexts to file~~ dump temporary security contexts to file for use in worker submit scripts Oct 20, 2021

riedel added bug Something isn't working needs triage This can be kept if the triager is unsure which next steps to take all job schedulers labels Oct 23, 2021

guillaumeeb reviewed Oct 25, 2021

View reviewed changes

Comment thread dask_jobqueue/core.py Outdated

riedel mentioned this pull request Oct 29, 2021

distributed.utils.tmpfile used in core.py to create the jobscript is deprecated #528

Closed

riedel marked this pull request as draft October 31, 2021 06:41

riedel removed the needs triage This can be kept if the triager is unsure which next steps to take label Oct 31, 2021

riedel mentioned this pull request Oct 31, 2021

In memory security contexts are not supported #520

Closed

riedel added the blocked Awaiting the output of some other work label Oct 31, 2021

riedel added 2 commits November 27, 2021 21:03

dump temporary security contexts to file

a8b35f5

move to temp file to predefined shared directory

c7174bb

riedel force-pushed the in_memory_security_contexts_520 branch from 8cd6a59 to c7174bb Compare November 27, 2021 22:11

riedel added 2 commits November 28, 2021 10:38

add shared_directory to tests and fail on using security if not set

2cf5d8b

explicitly set shared directory for test

f2cfcae

set the prefix of the temporary to be more readable

d7694c0

riedel added 2 commits November 28, 2021 11:58

clean up temporary file in context handler and test for cleanup

d0e120a

closing cluster in context handler and fix parallel testing and pbs c…

86977af

…i user * added context handler to Job Cluster in core.py * make source dir owned by pbsuser * dynamically select port to fix sporadic port already in use errors

riedel force-pushed the in_memory_security_contexts_520 branch from d7ade92 to 86977af Compare November 28, 2021 17:48

revert cleanup using context manager

36a4686

riedel changed the title ~~dump temporary security contexts to file for use in worker submit scripts~~ dump temporary security contexts to file in specified shared directory for use in worker submit scripts Nov 28, 2021

riedel force-pushed the in_memory_security_contexts_520 branch 2 times, most recently from 730cf43 to 1fdbe1d Compare November 29, 2021 11:58

refactor into function

bf0c957

riedel force-pushed the in_memory_security_contexts_520 branch from 1fdbe1d to bf0c957 Compare November 29, 2021 12:23

remove unused dependency

fbdbcf9

riedel commented Nov 29, 2021

View reviewed changes

Comment thread ci/pbs.sh

Comment thread dask_jobqueue/core.py Outdated

Comment thread dask_jobqueue/core.py

Comment thread dask_jobqueue/core.py

Comment thread dask_jobqueue/jobqueue.yaml Outdated

riedel requested a review from guillaumeeb November 29, 2021 12:36

andersy005 reviewed Nov 29, 2021

View reviewed changes

Comment thread dask_jobqueue/core.py Outdated

guillaumeeb reviewed Nov 30, 2021

View reviewed changes

guillaumeeb mentioned this pull request Nov 30, 2021

added xfailing test for known temporary security bug #537

Closed

riedel added 2 commits December 3, 2021 23:18

rename setting to shared_temp_directory and rewrote defaults and exce…

547afd2

…ption handling

revert change in htcondor

89b9820

because it xfails anyways and the problem with the ci is unrelated

riedel changed the title ~~dump temporary security contexts to file in specified shared directory for use in worker submit scripts~~ dump temporary security contexts to file in specified shared temporary directory for use in worker submit scripts Dec 3, 2021

riedel added 2 commits December 3, 2021 23:46

forgot to set extras_required

aecfcd0

tests_required is deprecated but kept as in distributed

fix incomplete comment

18925dc

guillaumeeb reviewed Dec 4, 2021

View reviewed changes

Comment thread dask_jobqueue/core.py Outdated

guillaumeeb approved these changes Dec 4, 2021

View reviewed changes

riedel mentioned this pull request Dec 4, 2021

add temporary security if set to True #527

Merged

Fix comment

a7d18bb

Co-authored-by: Guillaume Eynard-Bontemps <g.eynard.bontemps@gmail.com>

riedel commented Dec 4, 2021

View reviewed changes

Comment thread dask_jobqueue/core.py Outdated

riedel added 3 commits December 4, 2021 10:32

fixed error in check

6340a7f

Merge branch 'in_memory_security_contexts_520' of github.com:riedel/d…

a75470b

…ask-jobqueue into in_memory_security_contexts_520

fixed formatting

18da416

riedel force-pushed the in_memory_security_contexts_520 branch from d70544e to 18da416 Compare December 4, 2021 09:57

guillaumeeb merged commit 6ce5a6c into dask:main Dec 4, 2021

riedel deleted the in_memory_security_contexts_520 branch December 4, 2021 14:06

Uh oh!

Uh oh!

Conversation

riedel commented Oct 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

riedel commented Oct 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guillaumeeb commented Oct 25, 2021

Uh oh!

guillaumeeb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

riedel commented Oct 30, 2021

Uh oh!

riedel commented Oct 31, 2021

Uh oh!

guillaumeeb commented Nov 21, 2021

Uh oh!

riedel commented Nov 27, 2021

Uh oh!

riedel commented Nov 28, 2021

Uh oh!

riedel commented Nov 28, 2021

Uh oh!

riedel commented Nov 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

riedel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guillaumeeb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guillaumeeb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

riedel commented Dec 4, 2021

Uh oh!

guillaumeeb commented Dec 4, 2021

Uh oh!

riedel commented Dec 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

riedel commented Oct 20, 2021 •

edited

Loading

riedel commented Oct 20, 2021 •

edited

Loading

riedel commented Nov 28, 2021 •

edited

Loading