oref0
oref0 copied to clipboard
openaps
An improvement around the naming of the API_SECRET env var
The "API_SECRET" in root@rig:.profile is not the "API_SECRET" of at least 12 chars, it's the API_HASHED_SECRET.
It would be good to have the variable correctly named. Indeed, in oref0/bin/oref0-setup.sh we can see the awkwardness of this name:
LC_ALL=C grep -R API_SECRET . | grep HASH
./oref0/bin/oref0-setup.sh: API_HASHED_SECRET=${API_SECRET}
./oref0/bin/oref0-setup.sh: API_HASHED_SECRET=$(nightscout hash-api-secret $API_SECRET)
./oref0/bin/oref0-setup.sh: (cat $HOME/.profile | grep -q "API_SECRET" || echo export API_SECRET="$API_HASHED_SECRET" >> $HOME/.profile)
./oref0/bin/oref0-setup.sh: echo API_SECRET="${API_HASHED_SECRET}" >> $HOME/.bash_profile
./oref0/bin/oref0-setup.sh: (crontab -l; crontab -l | grep -q "API_SECRET=" || echo API_SECRET=$API_HASHED_SECRET) | crontab -
A fix for this would - in general - be a simple replacement of "API_HASHED_SECRET" in most instances of "API_SECRET" - though particular care would be needed in ./oref0/bin/oref0-setup.sh and in a few places in the docs.
The scope of this change seems well defined:
LC_ALL=C grep -Rl API_SECRET .
./docs/docs/docs/Customize-Iterate/autotune.md
./docs/docs/docs/Customize-Iterate/offline-looping-and-monitoring.md
./docs/docs/docs/Resources/troubleshooting.md
./docs/docs/docs/While You Wait For Gear/nightscout-setup.md
./oref0/bin/nightscout.sh
./oref0/bin/ns-dedupe-treatments.sh
./oref0/bin/ns-get.sh
./oref0/bin/ns-upload-entries.sh
./oref0/bin/ns-upload.sh
./oref0/bin/oref0-autotune.sh
./oref0/bin/oref0-dexusb-cgm-loop.py
./oref0/bin/oref0-ns-loop.sh
./oref0/bin/oref0-setup.sh
./oref0/bin/oref0-upload-entries.sh
./oref0/bin/oref0_nightscout_check.py
./oref0/lib/oref0-setup/alias.json
./oref0/README.md
Before doing anything else, I'd first like to ask whether a PR for this would be looked at? There might be longer-term plans re: securing Nightscout and/or rig comms that might void trying this fix.
The API_SECRET is exposed to users through their command-line env and through crontab, so any change there would cause confusion. Is there any substantive benefit to changing this?
The /value/ that's exposed through crontab, .profile and therefore the command-line env is the API_HASHED_SECRET, not the API_SECRET itself. In the future I'd like to take a stab at updating the docs to use "${NIGHTSCOUT_HOST}", "${API_SECRET}" and "${API_HASHED_SECRET}" consistently.
I know that existing OpenAPS users would need to be notified, and there'd be some thinking to do about releasing this, but we'd a better-named variable which would be easier to reuse afterwards.
If the only advantage is consistency and simplification, but it requires existing users to change things, I’d prefer to save the breaking change for the next time we have a more concrete reason to change that part of the system. The only thing I can think of along those lines is figuring out how to ensure that everyone’s API secret is a system-generated long random string, rather than a user-generated password.
I agree, I got confused by this when I installed my rig for the first time recently. This simple rename would have avoided being puzzled and would clarify things for me. +1.
It is a slightly non compatible change so we can be careful there.
I'm torn - but I understand that getting existing users to change things is not going to be a good ask. Is there a way that this could be flagged for the next significant upgrade? Not sure if going to 0.7 is "significant enough"? oref1 though surely?
If we're to do significant work with this, from Nightscout perspective the "correct" path would be to throw both API_SECRET and API_HASHED_SECRET away and move to the token based authentication...
We can support the old or new variable name in the bash profile until people migrate. Moving to token based is probably the best, but needs work in different places such as xdripaps & others.
I think it's necessary that we have backwards compatibility :-o @sulkaharo I'm not sure that this is "significant" work, but I wouldn't quote myself on that just yet ;-) Perhaps I'll take some time, build a branch with the proposed changes (I'd like to learn more of the code anyway) and we can then look at whether or not it's a big deal. I do acknowledge that this is unlikely to land anywhere unless we're/I'm unusually lucky with the backwards-compatibility of it.
@sulkaharo Where might I find out about Nightscout's token-based auth? Apart from setting up a NS for OpenAPS, I don't know much about it (yet).
@endafarrell How to use Nightscout token-based auth for openaps/oref0 is documented here: http://openaps.readthedocs.io/en/master/docs/While%20You%20Wait%20For%20Gear/nightscout-setup.html#switching-from-api-secret-to-token-based-authentication-for-your-rig If you have problems with it, please let me know (I implemented the tokenbased authentication in oref0).
@alimhassam @endafarrell @scottleibrand @sulkaharo @endafarrell :
Current token based auth implementation for oref0 uses the API_SECRET field with a token= prefix, followed by the subject name (e.g. rigname) and a secret. A better name for it would be NIGHTSCOUT_TOKEN.
For our upgrade path we should make a distinction between backwards compatibility:
- backwards compatibility for oref0 users using Nightscout API_SECRET instead of Nightscout token based authentications Users are familiar with
NIGHTSCOUT_HOSTandAPI_SECRETconcepts (and are often not aware of the problems with the API_SECRET_HASHED). - backwards compatibility for other diabetes apps suchs as xdripaps https://github.com/openaps/oref0/issues/651 , urchin cgm https://github.com/mddub/urchin-cgm/issues/64 and others diabetes apps that don't support token based authentication yet.
The upgrade path for issue 2 (other diabetes apps that don't support token based authentication yet), can be made easier by extending Nightscout to also allow to use tokens to authenticate to Nightscout (e.g. https://[email protected] ) and then get the permissions that belong to myrigname (instead of getting all permissions). I think it's possible to implement a backwards compatibility layer in Nightscout so that you can also authenticate for a specific role (and not by using ?token=myrigname-27c914cabc506fa3 at the end of the URL). In this case using Nightscout API_SECRET still can work, and will give the rig administrator privileges within Nightscout.
I think the best upgrade path for oref0 is to switch to token based authentication by default in a next major release. I think we can make that upgrade path easier by defining the oref0rig role by default in in a next Nightscout release. I think it's perfectly explainable to tell users to upgrade their Nightscout first, and then their oref0 rig. From oref0 0.7.x we can then ditch API_SECRET in oref0 and only use NIGHTSCOUT_HOST and NIGHTSCOUT_TOKEN in oref0.
If we want backwards compatibilty for issue 1 ( I'm pretty sure @scottleibrand would like that) we can adjust oref0-setup.sh to have a fallback that users can still enter their Nightscout administrator API_SECRET, and we will use that. We will store the hashed version of that in NIGHTSCOUT_TOKEN.
I'm happy to work on patches, if we agree to a common upgrade path for oref0 and Nightscout.
