15:09:55 <sdoran> #startmeeting Ansible Core Public IRC Meeting https://github.com/ansible/community/issues/507
15:09:55 <zodbot> Meeting started Thu Dec  5 15:09:55 2019 UTC.
15:09:55 <zodbot> This meeting is logged and archived in a public location.
15:09:55 <zodbot> The chair is sdoran. Information about MeetBot at http://wiki.debian.org/MeetBot.
15:09:55 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic.
15:09:55 <zodbot> The meeting name has been set to 'ansible_core_public_irc_meeting_https://github.com/ansible/community/issues/507'
15:10:00 <shertel> Hi
15:10:21 <resmo> hi
15:10:28 <keylemon> hello
15:10:57 <sdoran> #chair shertel
15:10:57 <zodbot> Current chairs: sdoran shertel
15:11:05 <sdoran> #topic https://github.com/ansible/ansible/pull/55268
15:11:30 <sdoran> Is evitalis around?
15:11:39 <keylemon> i am here, forgot to include my id in the issue sorry
15:11:40 <sdoran> We tried to discuss this last time but I don't think it went anywhere.
15:11:49 <sdoran> Ah, no worries.
15:11:54 <sdoran> You have the floor.
15:12:31 <keylemon> thanks, first meeting i have attended. i wanted to discuss https://github.com/ansible/ansible/pull/55268 which seems to have stalled out and got changed from my original pr.
15:12:54 <sdoran> I'm reading up on the issue.
15:14:28 <shertel> webknjaz, just in case you're around and free to take a look at the last comment
15:14:37 <keylemon> openbsd uses a the $2b designation for bcrypt which is an updated version solving for some issues in implementation. linux doesn't natively support bcrypt which is why i think the tests fail but am not sure.
15:15:13 <sivel> I'm concerned with portability
15:15:36 <sivel> what if you are on a team, and 1 person is using linux and another OpenBSD? Running the same playbooks
15:15:45 <sdoran> #chair sivel
15:15:45 <zodbot> Current chairs: sdoran shertel sivel
15:15:53 <sivel> building hashes differently due to special casing this way
15:16:05 <sdoran> Yeah, this is really tricky.
15:16:28 <sivel> Also, we have communicated before that the difference between 2a and 2b isn't really hugely important. I might have my response saved somewhere
15:16:30 <sivel> let me see
15:16:37 <sivel> it also included how we might go about allowing it
15:16:38 <keylemon> yeah i have that response as well
15:16:55 <sdoran> https://github.com/ansible/ansible/issues/54071#issuecomment-474616949
15:17:10 <sivel> Yes, that is my saved response
15:17:18 <sivel> to quote:
15:17:20 <sivel> > We would still be open to accepting a change which modified the passlib code path to use 2b instead of 2a.
15:18:06 <sivel> so our previous decision was to allow this change, but to make it only possible when using passlib
15:18:43 <sivel> and with that, I am sorry, but I must leave for another meeting
15:19:31 <keylemon> so should i open a pr with passlib or is there some other change to my pr here? i am not really sure how to fix it based on the conversation in the pr unfortunately
15:20:10 <keylemon> also its my understanding that the platform module may be deprecated for this use case
15:20:55 <sivel> There are 2 code paths in that file, one that uses crypt, another that uses passlib
15:21:13 <sdoran> Alright, so keylemon: it seems the the change to using `2b` should only be if using `PASSLIB_AVAILABLE=True`.
15:21:19 <sivel> PasslibHash would need to be updated to update the `algorithms` to specify 2b
15:21:39 <sivel> so that 2b only affected the PasslibHash class
15:22:39 <keylemon> got it. i think i  see where to change it now thank you sivel . should i close this pr and just open a new one with the change in the right spot?
15:22:52 <sdoran> You can just change the existing PR.
15:23:55 <sivel> and it need not be specific to OpenBSD, should just generically apply
15:24:33 <shertel> +1
15:24:33 <sdoran> #action keylemon to update logic in PR #55268 so it only affects the `PasslibHash` class and is not OpenBSD specific.
15:24:46 <sdoran> keylemon: Anything else on that topic?
15:24:47 <keylemon> makes sense. how should i handle the existing 9 commits in this case? to make sure i am consistent with the project
15:24:55 <keylemon> just that last question
15:25:35 <sdoran> You can leave them. We squash before merging into `devel`.
15:25:43 <keylemon> ok thank you. that is all for that pr
15:25:49 <sdoran> #topic https://github.com/ansible/ansible/pull/65112
15:26:36 <keylemon> on 65112 i talked to acozine the other day in #ansible-docs . it doesn't appear the logic broke it since if i revert the shell the ci reports the same errors
15:27:13 <keylemon> i also cannot seem to reproduce the ci failure locally
15:27:56 <sdoran> You could try running it using the container that we use in CI.
15:28:21 <sdoran> `ansible-test sanity --docker default --test docs-build`
15:28:28 <agaffney> that /usr/bin/command doesn't seem to exist on Linux, which makes it return non-zero and do the SHA_CMD=sha1 assignment, which seems to be backward from how it should work
15:28:58 <agaffney> I'm not sure why that '/usr/bin/command -v' part is needed in there at all
15:29:24 <sdoran> Maybe that's a BSDism?
15:29:27 <keylemon> i do see /usr/bin/command on my fedora workstation. but i can drop that check
15:29:33 <sdoran> Or a way to not use the shell built in `which`?
15:30:02 <agaffney> 'command' seems to be a shell built-in in bash, but there's no 'command' binary in Ubuntu
15:30:11 <keylemon> i looked it up and yeah it was a way to use the true command and not get mixed up with a built in. easy enough to remove and just let it use whatever for `which`
15:30:53 <sdoran> That's probably the most portable, though I understand trying to avoid using the shell builtin.
15:31:02 <sdoran> Some shell don't have it.
15:31:26 <sdoran> So try changing that test as well as trying to duplicate the failure locally using the CI container.
15:31:29 <sdoran> Maybe that will help.
15:31:45 <keylemon> sure both should be quick fixes and tests.
15:31:51 <sdoran> Anything else on this topic?
15:31:58 <sdoran> You can stop by #ansible-devel if you run into issues
15:32:25 <keylemon> it should be good to go once the ci passes or shouls i make any other changes?
15:33:05 <keylemon> otherwise that is all on that topic
15:33:07 <keylemon> thank you
15:33:29 <sdoran> I don't really know anything about that script. I would yield to acozine for final approval.
15:33:52 <keylemon> ok, thank you
15:33:57 <sdoran> You're quite welcome.
15:34:03 <sdoran> #topic https://github.com/ansible/ansible/pull/64436
15:34:28 <agaffney> that PR was merged already
15:34:39 <shertel> it looks like felix is not around to discuss this today
15:35:22 <sdoran> agaffney: It's merged in `devel`. nitzmahone came across it in the 2.9 backports and would like to discuss possibly reverting it (the backports have not been merged)
15:35:25 <shertel> may be a bit early for nitzmahone too
15:35:42 <sdoran> Yup.
15:35:53 <sdoran> Here's the comment from nitzmahone : https://github.com/ansible/community/issues/507#issuecomment-561384958
15:37:56 * nitzmahone blinks
15:39:00 <sdoran> Seems like this needs to be opt in and the existing dangerous behavior, as described by felixfontein, should be removed.
15:39:13 <nitzmahone> I'm uncomfortable with automatically blowing away perceived "invalid" keys by default. IMO there needs to be "force_regen_invalid" flag or something
15:39:19 <sdoran> > Finally, this can only lock you out of systems if you let the module operate on the only copy of the private key you have (i.e. you don't have any backup).
15:39:25 <sdoran> No one ever has backups. :)
15:39:36 <sdoran> (That's a quote from felixfontein )
15:39:43 <nitzmahone> Esp when the keys are being generated by the playbook :)
15:40:17 <shertel> do we want to change openssl_privatekey to be opt-in as well? Seems like it was done somewhat for consistency.
15:40:37 <nitzmahone> I'd be +1 for that
15:40:49 <nitzmahone> It seems just as dangerous
15:41:07 <sdoran> Yup.
15:41:22 <shertel> Using opt-in only seems like a good thing here. I'm +1.
15:41:23 <sdoran> I would say being consistent with the less-than-ideal thing is not good.
15:41:36 <nitzmahone> Ayup
15:42:06 <webknjaz> shertel: yes, I saw that but have no idea how to check that, I wish I could easily inspect all those OSs
15:42:37 <sdoran> I'm +1 for making this opt-in as well.
15:43:42 <shertel> webknjaz, sounds like we have a good solution :-) sorry for  the unnecessary ping.
15:44:20 <webknjaz> I only noticed it now. Make sure to post that solution back to the PR
15:45:27 <sdoran> #action revert commit da73bbd73c94b6bd5cc459f2e813b11014e44a7e
15:45:45 <sdoran> #action change behavior from PR #64436 to be opt-in
15:45:49 <shertel> the meeting logs will probably suffice. The proposed solution from sivel was already discussed in the pr/issue and just clarified here.
15:45:55 <nitzmahone> And I'll zap the backports
15:46:01 <sdoran> 👍
15:46:11 <sdoran> #topic open floor
15:46:17 <agaffney> https://github.com/ansible/ansible/pull/65487
15:46:53 <sdoran> #topic https://github.com/ansible/ansible/pull/65487
15:47:12 <nitzmahone> +1 from me
15:47:26 * sdoran looking
15:47:39 <agaffney> my only real question is if that's the best place to do the check, but I didn't see a better place
15:48:51 <nitzmahone> It's the best place right now... There's something a few of us have been kicking around that might make a better place someday, but as you know, you don't want to wait for it ;)
15:49:41 <shertel> shipit
15:50:31 <sdoran> My only concern is this could break playbooks that are silently ignoring these keys now.
15:50:41 <sdoran> Does this cause the play to fail?
15:50:56 <agaffney> yes
15:51:00 <nitzmahone> Could warm into it with deprecation, but I think that's an ok thing to break right away
15:51:22 <nitzmahone> "your syntax was bad, we just noticed"
15:51:49 <agaffney> this is no different than previously introduced errors for unsupported fields on include_* and such, imo
15:52:05 <sdoran> 🤔
15:52:08 <nitzmahone> I can't think of a legit purpose for allowing the badness
15:52:18 <agaffney> and it's not a deprecation, because it never worked
15:52:33 <agaffney> it was a silently ignored error before
15:52:50 <nitzmahone> (deprecation being "invalid fields warning, we're gonna break later")
15:53:05 <sdoran> I always just like to weigh introducing immediate breakage, even if it is for something that is totally busted.
15:53:20 <agaffney> I made it a failure for consistency with the existing check for the 'name' attribute
15:53:51 <nitzmahone> But I personally don't think that's necessary since it's always been a bug that we didn't validate that, and there's no legit purpose I can think of to allow bad syntax there
15:53:56 <agaffney> it's also really easy to fix your playbook to get rid of the failure, and that fix would be backward compatible (since it never worked anyway)
15:54:09 <nitzmahone> --^ this
15:54:17 <sdoran> Ok.
15:54:37 <sdoran> Merged.
15:54:43 <sdoran> #topic open floor
15:55:28 <nitzmahone> (and sdoran: keep weighing- it's important to avoid breaking stuff in most cases :) )
15:55:50 <sdoran> Yup. I just like to pause a reflect for a moment.
15:55:56 <sdoran> It's good mental exercise. :)
15:56:02 <nitzmahone> + 1000
15:56:28 <agaffney> indeed
15:56:39 <sdoran> I try to remember that one tiny decision affects millions of people now. 😬 But I don't dwell on it.
15:56:58 <nitzmahone> Because I'd definitely agree we've been a little cavalier about that on occasion on the past
15:57:12 <nitzmahone> *in
15:59:53 <sdoran> Thank you everyone for participating today.
16:00:01 <sdoran> #endmeeting