Skip to content
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

Specify (non-variable) tempfile name #190

Closed
OJFord opened this issue Jan 20, 2021 · 7 comments · Fixed by #201
Closed

Specify (non-variable) tempfile name #190

OJFord opened this issue Jan 20, 2021 · 7 comments · Fixed by #201

Comments

@OJFord
Copy link

OJFord commented Jan 20, 2021

Is your feature request related to a problem? Please describe

I would like to be able to specify in my ~/.ssh/config something like:

Host foo.example.com
    IdentityFile ~/.ssh/.identities/foo.example.com

and have summon populate the contents of .identities/foo.example.com (from gopass in my case).

Describe the solution you would like

I'm imagining something like:

summon --yaml 'FOO: !file=<path> contents'
summon --yaml 'FOO: !file=<path> $env/aws/ec2/private_key'

Describe alternatives you have considered

I suppose:

summon [...] sh -c 'ln -sf ~/.ssh/identities/foo.example.com "$FOO" && ssh [...]'

Additional context

.ssh/config files allow only certain environment variables and only for certain options, so IdentityFile $FOO wouldn't work, it would expect a file named literally '$FOO'.

@BradleyBoutcher
Copy link
Contributor

Hi @OJFord, thanks for using Summon, and thanks for submitting this request!

Currently, when Summon returns a filepath, it behaves idempotently. So, if we run a request with !file multiple times, we don't have to worry about secrets being stored long-term on disk, or files being overwritten. I think that's my main concern with being able to specify a filepath, in that it would break idempotency.

I have two questions:

  • Can you suggest how this might work when you run the summon command multiple times with the file path specified?
  • How can we be sure the secret will be removed from the file afterwards, so it isn't stored on the disc long term?

As with many Summon feature requests, we try to be sure that security is at the forefront of the product, and your alternative does seem like a better way to accomplish this request without making Summon itself potentially causing security vulnerabilities.

@diverdane
Copy link
Contributor

diverdane commented Jan 22, 2021

@OJFord,

Thank you for proposing this feature. I think this is feature would be useful enough
to consider, if we can resolve the concerns that @BradleyBoutcher brings up.

Thinking through @BradleyBoutcher 's questions, and to keep the conversation going...
I think we could potentially be okay from a security/idempotency perspective:

  • How this might work when you run the summon command multiple times:
    I think the concern is running Summon multiple times concurrently. Normally, each
    invocation of Summon will clean up its temp file when the subcommand that
    it's running is complete, at which point Summon exits. If we support a (fixed)
    file path to store secrets content,
    we don't want one Summon invocation to delete the file while another Summon
    invocation is using it. I think one way to handle this would be to have Summon
    NOT automatically delete the target file when a fixed path is being used.
    We would have to add clear documentation saying that cleanup of these
    fixed destination path files will need to be done manually, if desired,
    and that you may want to limit these files to root access only.
    (Or maybe add a summon command line option to determine auto-cleanup or not
    for the fixed-path files.)
  • How can we be sure the secret will be removed from the file:
    I think we'd have to have clear documentation that for !file=<path> variable specs,
    manual deletion of the files would need to be done if/when desired.

@diverdane
Copy link
Contributor

cc: @andytinkham

@diverdane
Copy link
Contributor

Just another thought, maybe we could explicitly document that running
concurrent instances of Summon that use the same fixed-path file is not inherently supported,
and delete the fixed-path files when Summon exits as it does for temp files.

@andytinkham
Copy link
Contributor

The latter path seems like a better case of failing safe. My initial reaction is that I'd prefer to see us clean up and document parallel execution is not supported rather than leave secrets lying around.

@rpothier
Copy link
Contributor

rpothier commented Jan 29, 2021

Hi @OJFord, Your alternative seems to be a good approach and does work in my testing.
Can you expand on the comment in "Additional context" of what issues you are seeing?
With my testing
summon [...] sh -c 'ln -sf $FOO foo.example.com && ls -l foo.example.com && cat foo.example.com && rm foo.example.com
seems to work as expected.

@OJFord
Copy link
Author

OJFord commented Feb 14, 2021

@rpothier I didn't mean my comment in 'Additional context' as a caveat to the 'alternative I had considered' - it was just context for why supplying an environment variable to the target process (e.g. ssh) isn't always sufficient (resp. because it won't interpolate them).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants