v1.0 discussions

[michaelwittig]

So this project got a bit messy because of me not merging stuff fast enough. I would like to get this project back on track. So let's discuss how we continue @shinenelson, @mvanbaak, and @dylansmith

What we have:

• Multi Account functionality: Allow getting users and keys from another AWS account #22 already as a feature issue here Use IAM accounts from another AWS account #21
• Use IAM Groups to manage access: we have quiet a few implementations here
  *Authorizing users based on AWS IAM User Groups #23 Downside, slower because group membership is checked on login, but we don't get into problems with out of date group memberships
  • option to limit access to an IAM group instead of all IAM users #19 does not handle changes in the group memberships?
  • @shinenelson import-user-from-groups branch should work similar than

And we also have a big PR #24 which contains basically all of the above functionality.

My suggestions for v1.0:

• merge Multi Account functionality Allow getting users and keys from another AWS account #22 and close Use IAM accounts from another AWS account #21
• merge Authorizing users based on AWS IAM User Groups #23
• close option to limit access to an IAM group instead of all IAM users #19 and Merge of my AssumeRole branch, and some other forks #24
• fix issue SaveUserName must be Unsave in authorized_keys_command #27 (fix is part of Merge of my AssumeRole branch, and some other forks #24)
• verify that Documentation is up to date
• verify that iam_ssh_policy.json is up to date
• verify that showcase.yaml is up to date
• verify that install.sh is up to date
• use install.sh in showcase.yaml to not duplicate code
• implement a test that ensures that the available features are working as expected

My suggestion for the future v1.x:

• implement a the better management of local users
  • uuid
  • local groups
  • shell etc etc
• improve performance of login procedure

What's your opinion?

[dylansmith]

I'm fine with whatever you all decide - I've moved to using SSH CA instead so I'm not impacted by the decision.

[mvanbaak]

I know my PR is huge. Sorry for that ;-) And I was already expecting it too invasive to be merged as-is, but wanted to be a good citizen and provide it anyways.

The suggestions look good.

If you want I can quickly make a PR for issue #27 alone so it's easier to merge.

The part where the users are deleted if they no longer are member of a group that should access, is something I would personally would like to see in 1.0. I cant remember exactly who's fork I took most of the work from, but I really liked it.

All in all I would say: thanks a lot and good to see you want to take this one further.

[michaelwittig]

A PR for #27 would be great 👍🏻

…

Am 29.03.2017 um 10:11 schrieb Michiel van Baak ***@***.***>: I know my PR is huge. Sorry for that ;-) And I was already expecting it too invasive to be merged as-is, but wanted to be a good citizen and provide it anyways. The suggestions look good. If you want I can quickly make a PR for issue #27 alone so it's easier to merge. The part where the users are deleted if they no longer are member of a group that should access, is something I would personally would like to see in 1.0. I cant remember exactly who's fork I took most of the work from, but I really liked it. All in all I would say: thanks a lot and good to see you want to take this one further. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[mvanbaak]

Another item that could go on the 2.X list:

• Make a single script which can handle both syncing the users as well as getting the key.

So for example: ssh-iam-check syncusers and ssh-iam-check checksshkey
The main reason for this is that we start to duplicate code in both sh scripts now (iam assumerole, probably later the group permission thing, and possibly others in the future)

Just a thought I had this morning while resolving conflicts on my outstanding PR's ;-)

[shinenelson]

IMO, we should merge #24 and close all the other minor pull requests. #24 has implemented all of them in a single pull request and the code looks consistent.

@mvanbaak has done a good job with his code quality. It's very easy to understand what is what and how. And the features are all well modularized too. That's one reason I said that we should merge #24 and not the other minor pull requests which doesn't have any modularization. Going forward, this functional approach would make this project easily extensible. Otherwise, the codebase will just get messed up and cluttered and non-understandable.

I just did a complete code review and everything should work fine as expected. 👌 👍

[mvanbaak]

Thanks for the review and the nice words. As I said the code is taken from some forks and I cleaned up/rewrote the things here and there to make it easier to understand.

As much as I would like to see it merged its up to @michaelwittig to choose.

I do however agree that it will be easier to maintain as things are split up in logical functions

[michaelwittig]

Ok, I'm fine with the decision to merge #24. I will work on the following items this week:

• verify that Documentation is up to date
• verify that iam_ssh_policy.json is up to date
• verify that showcase.yaml is up to date
• verify that install.sh is up to date
• implement a test that ensures that the available features are working as expected
• release v1

[mvanbaak]

Cool! Thanks! If I can help with the items let me know :)

[michaelwittig]

Could someone review bd9ff5d ?

[mvanbaak]

Cool, you even created a testsuite! Only suggestion from my side is to comment out the sed lines to set the assumerole var in the showcase so it's consistent with the other optional var sed lines. Left a comment in the commit as well.

Good work. thanks a lot!

[michaelwittig]

@mvanbaak I commented on your comment:) bd9ff5d#commitcomment-21694850

[michaelwittig]

v1.0.0 is released :)