trinsic-id / v2-sdk

Trinsic SDK repository
https://docs.trinsic.id/
Apache License 2.0
29 stars 15 forks source link

Profile Service - platform specific auth profile management #420

Closed tmarkovski closed 2 years ago

tmarkovski commented 2 years ago

Discussion Points

  1. Do we support user-specified profile save locations? (backend)
  2. Do we support OS-specific profile save locations? (backend)
  3. Support device-specific profile save locations (mobile)
  4. Should we allow the cli to "pollute" the expected profiles?
  5. Should we allow import/export of the cli profiles from the various languages? (backend)
  6. Should we specify the auth profile storage requirements in protobuf to ensure consistency across languages?
  7. Should we have a gRPC service for storing/aliasing profiles? (server)

Environments/Languages

Backend support:

Mobile support:

Frontend support:

sethjback commented 2 years ago

A couple of quick thoughts on this:

  1. (re 1, 2) I like the "well known" location we are using for file storage (i.e. the /~/.trinsic directory).
  2. (re 6, 7) Would it make sense to define a grpc service for this, and then have SDK specific implementations along with some common defaults. This would give us a standardized interface and payload that needs to be implemented, and we could, e.g. implement the file system storage, for each sdk as an example, but if other users had specific storage needs they could implement their own.
fundthmcalculus commented 2 years ago
  1. I like the "well known" location we are using for file storage (i.e. the /~/.trinsic directory).

Agreed, that said, we should support something else if the user decides. I think sane defaults are good, but user-control is always better.

  1. Would it make sense to define a grpc service for this, and then have SDK specific implementations along with some common defaults. This would give us a standardized interface and payload that needs to be implemented, and we could, e.g. implement the file system storage, for each sdk as an example, but if other users had specific storage needs they could implement their own.

I wouldn't be opposed to specifying the dataset in protobuf, so that it's consistent across languages for backend. For mobile, we should just do whatever that platform expects, since there are existing conventions for this type of storage.

tmarkovski commented 2 years ago

The profile data is already a proto message. We could also standardize the service API, though at this point it'll really be two endpoints Get(Name) and Set(Profile, Name). I don't know how many languages support custom local channels for this to help us be more productive, but definitely worth discussing if we plan to scale the local services.

My thoughts are supporting the following on the first phase:

  1. Should we allow the cli to "pollute" the expected profiles?

I think this will only affect dev machines. In all other cases, it may be useful for sysadmins to have the CLI available to perform manual tasks that match their server runtime profile. Otherwise, it should be easy to just specify different profile name to use.

The approach to this service should be interface based with default implementation based on the used platform and ability for users to provide custom implementation of the service. See my take on this for .NET.

How should this service be provided in the default service construction in a language idiomatic way? Ctor parameter? Global flag? DI override?

Should we blind the tokens before storage with a simple PIN to ensure it's not encrypted at rest, e.g. for browsers? This will add complexity to user flow, but we can also consider this at a later time.

fundthmcalculus commented 2 years ago

Thoughts

We could also standardize the service API, but ...

I like your style, but I think that's excessive standardization work for minimal return. Let's just have a "documentation handshake" contract to keep these largely the same, and not worry too much about it. Especially if we're going to have variations for web and mobile.

I don't know how many languages support ...

I don't think this should be a driving factor in the decision, and I would err on the side of consistency over convention. Let's follow the environment-approved methods on mobile and browser, and give more flexibility on the server/desktop side.

Supporting the following during the first phase

Clarify this (and the potential subsequent) phases please. Overall, I agree. I do believe we should allow the user to store information somewhere other than ~/.trinsic so that we can have portable installations. Definitely relates to the cli, and potentially usage on other environments.

"pollute" the expected profiles

See above, I think it's good to have the ability to do so, but be wary of defaults. If we are saving the account profile in a consistent format across languages (JSON or protobuf binary), then let's go ahead and make this an option (see comment about portable installation)

The approach to this service should be interface based with default implementation ... How should this service be provided?

Be aware that several languages we support don't actually have interfaces (Ruby, Python), and some others don't have inheritance the same way (Go). We can definitely do something similar with dependency injection across languages though it will be a different idiom for some. I am in agreement on the point of needing a mechanism for user-provided profile storage.

See my take on this for .NET.

Looks good to me, I've done something similar in a few languages as a prototype spike.

Should we blind the tokens before storage?

@sethjback what's your thoughts on this? I'm kind of ambivalent. Generally security is better, and we could make the Trinsic reference implementation do the blinding automatically (with provided key of course).

sethjback commented 2 years ago

We could also standardize the service API

I think was what I was thinking with having it as a separate, small, service definition. If it is self contained it could be isolated from other profile functions, and then anyone could implement a (class, interface, struct, insert language specific construct here) that provides the functionality.

If we abstract it beyond just profiles, it could form the foundation for all sdk related storage, and would allow, e.g., corporate users to implement a database storage solution.

Maybe this is over engineered, and I admittedly don't have as much context in the SDK as you all do.

The .NET implementation looks exactly how I would imagine it working. Also the golang SDK ProfileProvider interface - end users can implement that however they want if they don't want to user one of our implementations.

Be aware that several languages we support don't actually have interfaces

Agree that how it is implemented in each language sdk will be idomatic for the language. (e.g. in go, you could do the function options pattern and have something like withProfileStorage()).

While I think consistency is good, I think working with what is familiar to each language is important, and will ultimately make developers feel at home with using it.

Would this be another argument for using grpc to define the api?

Should we blind the tokens before storage?

ideally yes, but that doesn't necessarily mean it needs to be done via Oberon blinding too. I honestly don't think we can force this since each individual use case could be so different. I think this might be better handled in documentation around implementing / using the storage providers, and even in things like the cli giving (passive aggressive :-) ? ) warnings if an oberon token without the blinding flag is being saved. (Something along the lines of - oberon token being saved unencrypted, are you sure you want to do that?)

fundthmcalculus commented 2 years ago

I think was what I was thinking with having it as a separate, small, service definition. If it is self contained it could be isolated from other profile functions, and then anyone could implement a (class, interface, struct, insert language specific construct here) that provides the functionality.

Agree with this

If we abstract it beyond just profiles, it could form the foundation for all sdk related storage, and would allow, e.g., corporate users to implement a database storage solution. Maybe this is over engineered, and I admittedly don't have as much context in the SDK as you all do.

YAGNI, but I agree. Let's keep future expansion in mind, but not make it too complicated for now. I think the simple "allow the consumer of the SDK to inject their own storage solution via our interface [or language idiomatic construct]" is fine. I'd just create a sample test for this that saves the profile as XML or something to show the behavior.

Agree that how it is implemented in each language sdk will be idomatic for the language. (e.g. in go, you could do the function options pattern and have something like withProfileStorage()).

I'll be sure to lean on you for the gophering problem.

While I think consistency is good, I think working with what is familiar to each language is important, and will ultimately make developers feel at home with using it. Would this be another argument for using grpc to define the api?

Agreed, but where consistency can be achieved, that's a good thing. I don't want the SDKs taking on a life of their own if possible, especially while things are still in flux. As our product matures, we can do more in the SDK, taking into consideration what our users actually do. The problem with defining the api via GRPC is that the compiler generates client stubs that wouldn't be meaningful. I'd almost rather use openapi or something for this. Especially if it's simple, we can handle it manually.

ideally yes, but that doesn't necessarily mean it needs to be done via Oberon blinding too. I honestly don't think we can force this since each individual use case could be so different. I think this might be better handled in documentation around implementing / using the storage providers, and even in things like the cli giving (passive aggressive :-) ? ) warnings if an oberon token without the blinding flag is being saved. (Something along the lines of - oberon token being saved unencrypted, are you sure you want to do that?)

I'm all for a passive aggressive comment about security haha. I wouldn't force it to be sure, but maybe our default implementation for OberonFileProvider does so? Also, blinding method could be swapped out with a different provider as discussed above. Some of this is probably overkill for web/mobile, but certainly worth having on the serverside work.

tmarkovski commented 2 years ago

We already have SDK specific namespacing and proto for configuration options, we can continue using this for SDK specific data contracts.

Re: token protection, I think we start off with saving tokens unblinded and revisit that. Users still have the option to blind them using a single call, so we have that as advanced option for those that need it.

Profiles should be saved in a file that is the binary serialization of the AccountProfile message, or whatever message we agree on. One alternative is to wrap the AccountProfile in Any message, which will allow type name specification, for versioning scenarios once we get to that. This choice should be the same regardless of the storage option (filesystem, keychain, local storage).

Re: ruby / python lacking interface options - we'll just take the approach that makes sense for those languages to implement interface / contract pattern. We don't actually need to have interfaces if that doesn't make sense.

API Changes: This will change how we invoke the service constructions by passing the profile names, instead the actual profile. Additionally, we should probably decide what characters we allow in the profile name, that will be compatible with the underlying storage or we can use transformation function (hash?, character stripping) to generate file names / keychain entries.

michaeldboyd commented 2 years ago
fundthmcalculus commented 2 years ago

I think we start off with saving tokens unblinded and revisit that. Users still have the option to blind them using a single call, so we have that as advanced option for those that need it.

Agreed on this. @michaeldboyd let's make a note in the documentation that you can do this.

Profiles should be saved in a file that is the binary serialization of the AccountProfile message, or whatever message we agree on.

Agreed. I would use the proto version serialization to handle this. We can easily add some kind of upversioning code on the account profile to move from v1 to v2.

This will change how we invoke the service constructions by passing the profile names, instead the actual profile. Additionally, we should probably decide what characters we allow in the profile name, that will be compatible with the underlying storage or we can use transformation function (hash?, character stripping) to generate file names / keychain entries.

tmarkovski commented 2 years ago

I also propose we use a profile name called "default" by default. Initially, the SDK should be able to do do happy path. Ex:

Server context by provider:

Mobile context by user:

SignIn also takes an input profile name which will either GetOrCreate new auth profile and sign in to the correct ecosystem.

Open questions: how is the profile names passed to other services?

sethjback commented 2 years ago

Clarifying question: when CreateEcosystem is called, the profile that is returned is the ecosystem ID, which will then be used as the ecosystem the user is signing in to when they call Account.SignIn() correct?

What if a user has already signed in - does a call to Provider.CreateEcosystem() assume switching to that ecosystemID context, and would they need to sign in again, or would the SDK/Server "move" the wallet under any circumstances (for example if the wallet was in the __default ecosystem?

Think about how things like K8s do it - it is similar in that when you log in to a cluster they automatically add the token, etc. to the kube config and then switch to that context. If you want to add a cluster without logging in you can, but if you try to issue any commands against it they require to you sign in again.

tmarkovski commented 2 years ago

Clarifying question: when CreateEcosystem is called, the profile that is returned is the ecosystem ID, which will then be used as the ecosystem the user is signing in to when they call Account.SignIn() correct?

Correct, ecosystemId becomes part of the token. This will save a step checking this in the server. All data will be scoped to an ecosystem.

What if a user has already signed in - does a call to Provider.CreateEcosystem() assume switching to that ecosystemID context, and would they need to sign in again, or would the SDK/Server "move" the wallet under any circumstances (for example if the wallet was in the __default ecosystem?

Provider.CreateEcosystem() is unauthenticated endpoint, it's similar in function to account registration. Users will have to manage multiple profiles if they want to access multiple ecosystems. Additionally, under the new restructure, inviting wallets to an ecosystem, simply creates a new wallet under that ecosystem.

fundthmcalculus commented 2 years ago

Users will have to manage multiple profiles if they want to access multiple ecosystems

Isn't this going to end up with the same headache as users currently juggling multiple email inboxes? Is there any plan to support exporting credentials from one wallet to another that only the user controls?

tmarkovski commented 2 years ago

Yes, a long term plan. Users generally won't need to worry about multiple profiles, as they will be completely separate. Separate as your gmail and facebook login credentials, they're part of entirely different ecosystems.

sethjback commented 2 years ago

Provider.CreateEcosystem() is unauthenticated endpoint, it's similar in function to account registration.

How are we tying the creator of an ecosystem to the authz. Right now, when an ecosystem is create, the wallet making the call gets essentially admin/root privileges. If create is unauthenticated, how will we bootstrap the initial grants? Or will we just assume the first wallet to SignIn() under that ecosystem should receive the admin role?

tmarkovski commented 2 years ago

We create the wallet, create ecosystem, grant admin access to walletId for that ecosystem. Basically, we do the SignIn flow + CreateEcosystem flow.

sethjback commented 2 years ago

Just highlighting two points based on conversations this past week:

  1. Profiles should have an easy export and import functionality

Currently they are saved, e.g., by the cli in their binary proto serialization. This is convenient and compact in many cases but doesn't allow a "copy and past" type experience if you are moving them.

Should we implement something like a base58/base64 export?

  1. Segmenting authorized and un-authorized calls.

Most calls are authorized and thus need a profile to succeed. There are several (like signin and create ecosystem) that do not. Should we move all un-auth calls to their own service just to make the experience cleaner and more user-friendly?

tmarkovski commented 2 years ago

Re: 1. - what if we just replace AccountProfile with a string which would be a base64 encoded of the proto serialized value. This will make it easy for the token to be copy/pastable, but it'll also make the API slightly less confusing, since the AccountProfile type is something users shouldn't mess with.

fundthmcalculus commented 2 years ago

@tmarkovski @sethjback

  1. we should use base64 over base58, it's more commonly supported. I am in favor of this idea, since it makes token secrets easier to manage (plaintext).
  2. Unless the separation is a logical one, I wouldn't segment based on authentication. That seems like an arbitrary separation.
fundthmcalculus commented 2 years ago

@tmarkovski is this ready to be implemented, or should it be closed?

tmarkovski commented 2 years ago

I think few changes proposed here are now obsolete, and others we'll address in future. This can be closed.