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

Stale Project - Starting Collaboration? #61

Closed
patrickmelix opened this issue Oct 19, 2020 · 19 comments
Closed

Stale Project - Starting Collaboration? #61

patrickmelix opened this issue Oct 19, 2020 · 19 comments

Comments

@patrickmelix
Copy link

Hello everybody,

since it seems that @cornelinux does not have the time to keep working on this, I'm pinging here everybody with open MR (@superteece @finlstrm @pstray @erolg @Vincent43 @Gargravarr2112) and contributors (@scratchhax @CristianCantoro @michael-k @markchalloner @magenbrot @awnz @EdwardBetts).

My suggestion would be to create a fork of the repo where we can work on the open issues and merge requests. Then we contact the Debian maintainers to switch to this fork to get an updated version into the Debian universe. I would not want to do it alone and also future updates would benefit from some more people with write access to the forked repo.

Alternatively, @cornelinux could of course also decide to include more people with write access to maintain the repo in place?

Any suggestions welcome!
Cheers,
Patrick

@cornelinux
Copy link
Owner

Hi Patrick,
thank you for your energy!
What would be your most pressing pull request? Maybe we can get back on the road this way.
Kind regards
Cornelius

@FONDSMATIVE
Copy link

Hi Patrick,

thank you so much for taking over this great project!!!

Due to a lack of support of cryptsetup/askpass on CentOS 7 (see package Files here), I would love to point to following possibilities here. That would make the scripts more flexible on a lot more systems. It would be great to find a way, that is more 'common' without using askpass.

Furthermore I could hardly find any 'How-To's' for centos systems. It would be great if someone could point out for everybody else what to do to use the YubiKey as a second factor for Luks encrypted files on centos as well. This is missing as well.

Thanks again.
FONDSMATIVE

P.S:
Going down that path I did some work to use the scripts on centos as well - especially I worked on

  • yubikey-luks-enroll
  • yubikey-luks-open

What I changed is following in a 'quick' way. You could also use encpass.sh

vim yubikey-luks-enroll

P1=$(read -sp "Please enter the yubikey challenge password. This is the password that will only work while your yubikey is installed in your computer: ";echo $REPLY)
	if [ "$DBG" = "1" ]; then echo -e "\nPassword: $P1"; fi

P2=$(read -sp "Please enter the yubikey challenge password again: ";echo $REPLY)
	if [ "$DBG" = "1" ]; then echo -e "\nPassword: $P2"; fi

OLD=$(read -sp "Please provide an existing passphrase. This is NOT the passphrase you just entered, this is the passphrase that you currently use to unlock your LUKS encrypted drive: ";echo $REPLY)
	if [ "$DBG" = "1" ]; then echo -e "\nOld passphrase: $OLD"; fi

vim yubikey-luks-open

P1=$(read -sp "Enter password created with yubikey-luks-enroll: ";echo $REPLY)
	if [ "$DBG" = "1" ]; then echo "Password: $P1"; fi

@CristianCantoro
Copy link
Contributor

@FONDSMATIVE wrote:

Going down that path I did some work to use the scripts on centos as well - especially I worked on

* yubikey-luks-enroll

* yubikey-luks-open

What I changed is following in a 'quick' way. You could also use encpass.sh

This could be used as a fallback, what askpass does is (from its help) querying the user for a system passphrase, via the TTY or an UI agent, so it make sense to have a fallback that can be used on different systems.

@patrickmelix
Copy link
Author

@cornelinux wrote:

Hi Patrick,
thank you for your energy!
What would be your most pressing pull request? Maybe we can get back on the road this way.
Kind regards
Cornelius

Hello Cornelius,

thanks for getting back to this! I would of course say that mine is the most important, as I would otherwise not have implemented it 😉 No, seriously. Perhaps let's go first for the cleanup and style MRs (#49 #50). #43 looks like a no-brainer, so this might even be a good first.

Perhaps in a second step some people could test the MRs with new functionalities that only implement some minor features (#24 #52 #57).

The latest #60 is perhaps worth a bit more discussion, so perhaps later.

I'm happy to test changes on my platform if needed, just ping me.

Thanks for reviving this awesome piece of work!
All the best,
Patrick

@patrickmelix
Copy link
Author

@FONDSMATIVE, thanks for your enthusiasm. But I think this kind of discussion would be better suited to an issue or a MR. Let's first focus on how to get the things piled up done. Then we can discuss about new features and ideas where to go from there.

@cornelinux
Copy link
Owner

@patrickmelix thanks for sorting this out. I merged #43, #49, #45 Will continue with your help. Thanks.

@Vincent43
Copy link
Contributor

I recommend merging #63 too as it fixes a little security issue.

@patrickmelix
Copy link
Author

Looks good to me, but definitively clashes with #57, so we should decide which one comes first. Perhaps #63 and then I update #57?

@Vincent43
Copy link
Contributor

I wouldn't mind if my change goes first 😄 .

@cornelinux
Copy link
Owner

@patrickmelix @Vincent43 look at this #57 and #63 merged. (Hope I did not introduce an issue when resolving merge conflicts).
Thanks folks!

@patrickmelix
Copy link
Author

patrickmelix commented Oct 24, 2020

Wow, this is quite an impressive speed! Awesome. All changes work fine for me too. I guess the biggest one open is now #46.
Thanks a lot @cornelinux. Perhaps then it is also the time to create a new release, so it can be picked up by the distro maintainers?!

@cornelinux
Copy link
Owner

What should I do otherwise? Go shopping? No - this totally sucks nowadays ;-)

@Vincent43
Copy link
Contributor

Vincent43 commented Oct 24, 2020

You may consider radical but quite reasonable decision to remove suspend parts from this project as it needs a lot of love and I don't have time to help. There are many corner cases which breaks it which makes it hard to recommend for average user. Debian recently added similar functionality to cryptsetup package (for now only in experimental branch). This is supposed to work with tools like yubikey-luks called through keyscript. Sooner or later it should land in stable branches and the result should be better than whatever we may achieve here.

@cornelinux
Copy link
Owner

I made the suspend configurable. So you can run without it (if it does not work for you - like me).

@Vincent43
Copy link
Contributor

Vincent43 commented Oct 25, 2020

Technically speaking suspend was already configurable - it did nothing if you didn't explicitly enable it with systemctl enable yubikey-luks-suspend.service (which isn't run by default) and could be disabled with systemctl disable yubikey-luks-suspend.service afterwards. So before your change it was enough to run one command to enable/disable suspend while now you need to enable it in config first then rebuild initramfs which I think is regression for usability. Moreover if someone runs that command (which is still what readme says to do) and doesn't change the config plus rebuild initramfs it may break suspend altogether.

Also this command is for suspending system right away which I don't think you wanted there.

Last thing is that you should initialize variable with default value as it may not exist in sourced config in some circumstances.

@cornelinux
Copy link
Owner

I ran into an error on Ubuntu 18.04 when suspending if the suspend script was added to the initramfs.
THanks for the hint. Will look into this.

@Vincent43
Copy link
Contributor

Vincent43 commented Oct 25, 2020

This is worth investigating then but from pure logic if yubikey-luks-suspend.service isn't enabled then initramfs plays no part during suspend (it shouldn't exist at all as script that unpacks it is never called). How then initramfs content can affect suspend? Are you sure that you disabled yubikey-luks-suspend.service when you hit that error? In #62 you wrote The system does not accept the passphrase which means you had yubikey-luks-suspend.service enabled otherwise nothing would ask you for any passphrase as LUKS volume wouldn't be closed in first place.

I have Ubuntu 20.04 system and can't reproduce your issue.

@Vincent43
Copy link
Contributor

Here's fix for broken syntax introduced in enroll script: #64

@cornelinux
Copy link
Owner

I think the topic of 1FA, which was mentioned in #60 is quite nicely handled in the newer PR #65.

I am closing this issue, since it might get a bit cluttered.
Anyone feel free to open new ones.

@Roman-Koshelev Roman-Koshelev mentioned this issue Nov 9, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants