Custom auth fixes, tests, and CI framework #422
Conversation
bretambrose
changed the title
| @@ -31,77 +30,27 @@ env: | |||
| CI_FLEET_PROVISIONING_ROLE: ${{ secrets.AWS_CI_FLEET_PROVISIONING_ROLE }} | |||
| CI_DEVICE_ADVISOR: ${{ secrets.AWS_CI_DEVICE_ADVISOR_ROLE }} | |||
| CI_MQTT5_ROLE: ${{ secrets.AWS_CI_MQTT5_ROLE }} | |||
| CI_BUILD_AND_TEST_ROLE: arn:aws:iam::180635532705:role/V2_SDK_Unit_Testing | |||
There was a problem hiding this comment.
Minor: I'd prefer if this was hidden in a GitHub secret. Technically there is no reason to put it in a secret, as it cannot be used without authenticating the IAM role on a per-repo basis, but I feel some additional security through obscurity couldn't hurt. It also makes it easier to change the IAM role in the future without needing to make a commit/PR.
There was a problem hiding this comment.
Well I have the opposite conviction. I'm convinced we need to remove all Github secrets for maintainability reasons (do you know what IAM policy is bound to the credentials embedded in some of the repos? I don't and I have no way of finding out).
There was a problem hiding this comment.
Hmm, fair point. That is something I had not considered and I can see how having it hidden away in secrets would make it impossible to know exactly what credentials are bound to what IAM role. Over time, I could see this being an issue, especially as it is forgotten why/when/who setup what.
Hiding it in secrets provides only a tiny bit of extra security, so, there's that too. It does expose the AWS account ID, but I don't think there is any harm in that? I cannot think of any right off at least. Since the IAM roles cannot be used outside our repositories, I do not think there is any worries there either.
In that case though, should we change this for all the IAM role secrets in all 4 of the V2 IoT SDK repositories? I think we should be consistent with how we do it, and so if we want to not use GitHub secrets for IAM roles, then we should apply this change across the board in our repositories.
Obviously this is outside the scope of this particular PR, but if we want to take this approach, then I can make PRs to do this as I know what IAM roles go to what secrets.
There was a problem hiding this comment.
I think we should aim for no secrets and yes that requires quite a bit more work. I also think we should move tests away from requiring the test fixture to query secrets and instead make everything environmental injection via script from builder actions. That gives us a pattern that is the same across all repos, regardless of language, and the amount of resources and set up is significantly lessened.
There was a problem hiding this comment.
Okay. As a first step, I'll work on removing the IAM role secrets first 👍
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.