Security: App tokens #246
Security: App tokens #246
Conversation
core/keychain/app_token.go
Outdated
| err = ring.Set(keyring.Item{ | ||
| Key: AppTokenStoreKey + "_" + tok.Key, | ||
| Data: []byte(permissions.MarshalFullToken(tok)), | ||
| Label: "Space App - App Token", | ||
| }) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if tok.IsMaster { | ||
| if err := kc.st.Set([]byte(getMasterTokenStKey()), []byte(tok.Key)); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := ring.Set(keyring.Item{ | ||
| Key: getMasterTokenStKey(), | ||
| Data: []byte(permissions.MarshalFullToken(tok)), | ||
| Label: "Space App - Master App Token", | ||
| }); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
If tok.IsMaster is true, wouldn't this mean that we would store the token's information twice one for the "Space App - App Token" label and then for "Space App - Master App Token"? Is this the intended behaviour
--
Update: I see you tested for this exact case in the test cases, but still want to confirm
There was a problem hiding this comment.
Yea, wanted to access both master and regular tokens with the same logic, but at the same time needed some way to know if there was a master token present to prevent overwrites.
| err = ring.Set(keyring.Item{ | ||
| Key: AppTokenStoreKey + "_" + tok.Key, | ||
| Data: []byte(permissions.MarshalFullToken(tok)), | ||
| Label: "Space App - App Token", |
There was a problem hiding this comment.
Maybe we move this "Space App - App Token" string into a constant above as well. Same for the "Space App - Master App Token"
There was a problem hiding this comment.
I don't really think it's necessary as this is just so that it gets displayed like that when you browse them in the keychain. It's not like the key to access the data later on.
core/permissions/app_token.go
Outdated
| Permissions []string | ||
| } | ||
|
|
||
| // Token structure is [KEY].[SECRET].[IS_ADMIN].[PERMISSION1]_[PERMISSION2]... |
There was a problem hiding this comment.
Perhaps marshalling into a json string might be better and less error prone? Is there a reason why this format was chosen?
There was a problem hiding this comment.
I can't recall why I went for this approach but you're right, I'll update it to a json marshal.
* SentFile: use db.OrderByID() * Fix search db reinstantiation and mirror bucket creation failure (#253) * Security: App tokens (#246) * App token flow ready. Tests WIP. * Remove comment * Add tests * Fix github actions set-env deprecation Co-authored-by: maurycy <5383+maurycy@users.noreply.github.com>
Change log
Now all endpoints require the client to send an App Token, otherwise the request gets rejected.
Add
InitializeMasterAppToken, which returns a token with full access only if there's no other master token already created.Add tests
Add a stub for
GenerateAppToken. Didn't implement it yet (but the logic is there) since it will require some kind of "app store" view on the client so that users can selectively authorize apps with scoped access (e.g. Do you authorize X to ListDir and OpenFile?).Tested it on the client-side in this PR FleekHQ/space-client#102