medic / cht-android

A native Android container for Community Health Toolkit (CHT) applications
GNU Affero General Public License v3.0
25 stars 49 forks source link

CHT Android repository needs to find way around 100 secret/repo limitation #345

Closed mrjones-plip closed 6 months ago

mrjones-plip commented 7 months ago

Describe the issue MoH Togo can not publish their APK because this cht-android repo has hit the 100 secret/repo limit. They're looking for an immediate term work around for this:

Describe the improvement you'd like A long term solution so that future deployments have an easy way to publish their APKs. This might include:

Describe alternatives you've considered

kennsippell commented 7 months ago

There are 24 secrets for _ALIAS_ and _KEYSTORE_PATH_ information which I believe are totally unused and might provide months/years of needed secrets.

mrjones-plip commented 7 months ago

@craig-landry or @garethbowen - I can't see the secrets (not the values obviously, but the number of them and their names). I think you're both busy right now (this week?), but when you're "back in the office", can you look into a basic audit of the secrets we have in the repo?

Maybe getting rid of the 24 Kenn mentioned is enough to wrap this ticket up?

garethbowen commented 7 months ago

Another option is to use the 1pass API to store them there. We could have a vault dedicated to this.

can you look into a basic audit of the secrets we have in the repo?

I did a quick search on the code and I think those are referenced, eg: https://github.com/medic/cht-android/blob/5e64289fe74a125f0118cdd7daaaa04833ae1ccc/Makefile#L173-L174

However why this would ever be set to anything other than the default I don't know.

@kennsippell Any ideas? I'm guessing these secrets all have the same value so it'd be safe to delete?

kennsippell commented 7 months ago

Yes. Those two are used but the app specific ones I believe are unused and safe to delete. I think the keygen script outputs 6 secret values but only 4 of them are used.

garethbowen commented 7 months ago

Those two are used but the app specific ones I believe are unused and safe to delete.

The lines I quoted are the app specific ones, specifically ANDROID_KEYSTORE_PATH_${ORG_UPPER} so they are referenced in code. Do you think that block isn't running, ie, ifndef ANDROID_SECRETS_IV is false?

garethbowen commented 7 months ago

@mrjones-plip I think the way forward is...

  1. Modify the code above to use hardcoded paths for these two variables with the potential to overwrite with secrets to maintain backwards compatibility (just in case someone is changing it, potentially in a fork). I think hardcoded paths are better than secrets here because there's nothing that's secret about these values and it reduces complexity to have the values visible in the repo rather than obscured in config.
  2. If Kenn is right and the "keygen script outputs 6 secret values" then modify it to only output 4.
  3. Unfortunately if my understanding of the code is correct we still can't delete the unnecessary secrets because partner apps are on older versions of cht-android which depend on them, so for backwards compatibility, we'll leave the secrets there and clean them out when necessary, or at a certain time in the future to allow app services to migrate.

Can you prioritise this with Allies other work?

mrjones-plip commented 7 months ago

@garethbowen - yeah - sounds good!

Can you give me access to view secrets in CHT Android repo? While I know I can't retrieve the values here, I'll need to know which ones we've defined.

Do we know of the next CHT Android app that we'll need to create in terms of how pressing this is? (it's pressing I know, but HOW pressing ; )

kennsippell commented 7 months ago

The lines I quoted are the app specific ones

My mistake

garethbowen commented 7 months ago

@mrjones-plip I've made you a full admin on this repo.

mrjones-plip commented 7 months ago

Thanks for the perms @garethbowen! Confirmed I can now audit secrets.

There are currently 97 secrets and currently we need 4 per new app as I understand it:

Can you prioritise this with Allies other work?

@garethbowen - This seems like we need to drop everything and fix this ASAP, if only to find a stop gap solution for the next APK or two, if not a holistic long term solution. That sound about right?

garethbowen commented 7 months ago

@mrjones-plip It's one of those things that's not going to matter at all, until it matters a great deal. But the fix should be easy enough, right? The basic version is just replacing the lines in the makefile with hardcoded strings.

I don't think we need to "drop everything" unless you know of some project that's blocked on this (or soon will be). But getting it done this quarter would be good.

mrjones-plip commented 7 months ago

Awesome - thanks for the confirmation @garethbowen !

nydr commented 6 months ago

Another option is to use the 1pass API to store them there.

Example of how to use 1pass in gh-actions with a service account in case it's useful to anyone

kennsippell commented 6 months ago

@sugat009 I'm just going to keep this issue open to track actual deletion of the unneeded secrets in GitHub

kennsippell commented 6 months ago

@jkuester @sugat009 I've deleted unneeded secrets. There are now only 75 tokens in the cht-android repository. Enough to publish 12 more apps.