-
Notifications
You must be signed in to change notification settings - Fork 14
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
149 full disk luks encrpytion option for Installer #150
Conversation
Add optional settings to allow LUKS encryption of Rockstor OS disk.
@Hooverdan96 Nice addition, and well done on doing all the experimentation to get us this far. There is however a mixture of tabs and spaces! It probably doesn't matter but do you fancy correcting that? We likely already have the same elsewhere: and of course spaces is the way forward here :) . A follow-up commit would do so-as not to have the hassle of rebase etc. Plus once a force push has occurred on a PR our test backend often fails to be able to test the results there after! |
I'll take care of it, the mix of tabs and spaces. |
see whether that latest push addressed it. I think (though my eyes might deceive me) all my changes are spaces instead of tabs now. |
@Hooverdan96 I've just run a test build for the 15.5 x86_64 profile, with this PR pre-merged, and I get the following:
Not looked any further yet unfortunately. But did confirm an invalid XML for the resulting config file using an online XML linting tool. We could do with a basic format sanity checker for PR's in this repo actually. |
well, that's on me. I thought I could get away without doing a test build again after reformatting/commenting. thanks for keeping me honest. I will check into why this is happening and adjust accordingly. My apologies. For the XML-challenged like me, one cannot add a comment attribute within a tag (which I unfortunately did), only before or after. I will correct this. |
Well, it turns out it's still "complicated". In the
My workaround for this is to write the comment line like this:
which works while it's commented out (i.e. no error, and the iso is built), as well as when it's "active" (i.e. the LUKS based image is built and can be successfully installed and rebooted). Let me know whether that's acceptable? Anybody that wants to add other cryptsetup parameters and has them active, doesn't need to use the Unicode codes, but could use the double-hyphen ... this is only to enable the ability to have this tag commented out in the If this is acceptable I will push the updated file. I can also add a comment to the readme when discussing the LUKS setup and state that any additional parameters in that section can be stated with |
Spaces vs. Tabs for indentation Escape double-hyphen parameter within comment to avoid inconsistent XML format
@phillxnet, when you get a chance, see whether that works better. Before pushing I ran the tests as described above ... but since I flip between PC and Linux boxes there's always a chance that something gets messed up ... |
@Hooverdan96 Just re-tested this again and we have the following:
So it's a comment all-or-nothing setup currently. I'll push on with our existing installer release and we can fix this additional comment requirement there after. The XML format is quite picky all-in. Maybe once we are there on this one we can squash and re-present to avoid the development noise. If you fancy doing another commit here first re last issue I can re-run installer build to see if it's happy again before squashing. |
@phillxnet, apparently I pushed a kiwi file version where I had not commented out all LUKS related entries, no idea how that happened. In any case, as you can see, I rebased my branch to include your recently merged kiwi file version, and made the remaining adjustments. Hope, this time it's the charm. |
@Hooverdan96 I've just done a fresh TW X86-64 profile installer build and all is well now: as expected. Thanks again for looking into, and noting in comments, these LUKS options. Much appreciated. Merging pull request as-is give we are not too precious about git commit squashing in this repo given it's relatively slow turn-over. |
Thanks @phillxnet Leaving these two links behind here, in case a recent discussion about a LUKS keyfile/password bug will result in a change upstream that negatively impacts current approach to enable a LUKS encryption of the boot drive during the initial Rockstor setup: Reported SUSE issue upstream: https://bugzilla.suse.com/show_bug.cgi?id=1218181 |
Fixes #149.