passing args to 'cryptsetup open'

[somewhere-or-other]

Quick attempt to allow passing of arbitrary args to 'cryptsetup open' when unlocking volume. Related to the discussion here.

[somewhere-or-other]

This is just a quick, minimally-tested version, so feel free to give feedback here, if I've made any egregious errors, etc.

[somewhere-or-other]

I considered putting the new ${OPENARGS} in double-quotes, but was concerned about what would happen if someone wanted to include a space in their parameter string, like passing two different parameters (eg. -a -b, as a contrived example), or a space-separated name/value pair. From my tests, it seemed as though enclosing ${OPENARGS} in double-quotes, made cryptsetup consider it a single parameter that happened to contain spaces.

For my personal use, I only had one parameter to pass, so it would work either way. I just wanted to make a solution for a more general use case.

If there's a better way to handle this, I'm happy to take suggestions.

[sarroutbi]

Yes, I had not considered about the issues when using quotes. So change looks good to me.

[somewhere-or-other]

If I'm being honest, I did put it in quotes in a private/dev repo, and discovered the problem, before removing them, and subsequently squashing/rebasing everything into a single pull request.

[sarroutbi]

Nice. In my opinion, it can be merged, but let's wait for @sergio-correia to review it.

[sarroutbi]

As additional improvement, it would be nice to add a test to src/luks/tests to simulate a call to clevis-luks-unlock with a cryptsetup option

[somewhere-or-other]

@sarroutbi I've just submitted an initial attempt at a test. Basically it just uses a bash function to hijack cryptsetup temporarily, and verifies that the arbitrary value makes it through.

It's very possible I've done something incorrectly, though, so don't just take my word for it. Took me a while to figure out the testing/development environment. And that's why I'm mostly a sysadmin, not a developer.

[somewhere-or-other]

@sergio-correia @sarroutbi Is there anything I can do to shepherd this process along? It's not so urgent that it must happen now or anything. I just wasn't sure what kind of timeframe I could expect something to happen. It's fine if it takes a while, but it would be helpful to know, even roughly, how long it is likely to take.

[sarroutbi]

@sergio-correia @sarroutbi Is there anything I can do to shepherd this process along? It's not so urgent that it must happen now or anything. I just wasn't sure what kind of timeframe I could expect something to happen. It's fine if it takes a while, but it would be helpful to know, even roughly, how long it is likely to take.

Hello @somewhere-or-other. In my opinion, this could be merged, but I would prefer @sergio-correia to review it. By the way, thank you very much for adding a unit test associated to this new option.

[somewhere-or-other]

@sergio-correia, @sarroutbi

I realize it's been a while for this PR, but I'd still love to see it get merged, if possible. We're starting to see more and more use cases for it, via the related ClusterLabs PR. I'm happy to re-work it if there's a problem you can identify.

[sarroutbi]

Hello. Thanks for your PR. Changes LGTM. However, I miss including the option added to its appropriate manual page.

Could you please document new option in file src/luks/clevis-luks-unlock.1.adoc?

[somewhere-or-other]

@sarroutbi You're absolutely right. I totally forgot to do that. Should be there now.

[sarroutbi]

Changes LGTM

[somewhere-or-other]

I'm reviewing the logs from last night's failed build tests. Given that my test seems to have passed, and another one was the one that failed, this seems like a more general problem, either with the code base, or the testing infrastructure.

If I'm wrong, please let me know, and I'll do my best to fix it.

[sarroutbi]

I'm reviewing the logs from last night's failed build tests. Given that my test seems to have passed, and another one was the one that failed, this seems like a more general problem, either with the code base, or the testing infrastructure.

If I'm wrong, please let me know, and I'll do my best to fix it.

Let me re-run all the jobs and let's check. I agree this seems more related to testing infrastructure.

[sarroutbi]

@sergio-correia : please, review when possible. Changes LGTM.

[mvollmer]

This is super useful, but I don't see it in any release yet. Is that right?

[mvollmer]

@sarroutbi, @sergio-correia, could we get a release with this, please? So that this will eventually end up in distributions.

[sarroutbi]

Hello @mvollmer . We have some pending PRs that need to be reviewed. Once merged, we will create the new release.

[mvollmer]

Hello @mvollmer . We have some pending PRs that need to be reviewed. Once merged, we will create the new release.

Thank you!