Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add p/demo/authctx #1244

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from
Draft

feat: add p/demo/authctx #1244

wants to merge 13 commits into from

Conversation

moul
Copy link
Member

@moul moul commented Oct 15, 2023

@moul moul self-assigned this Oct 15, 2023
@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Oct 15, 2023
@codecov
Copy link

codecov bot commented Oct 15, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (89428c5) 47.84% compared to head (bf4fb3c) 47.86%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1244      +/-   ##
==========================================
+ Coverage   47.84%   47.86%   +0.01%     
==========================================
  Files         369      369              
  Lines       62764    62764              
==========================================
+ Hits        30028    30039      +11     
+ Misses      30308    30301       -7     
+ Partials     2428     2424       -4     

see 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

moul added 3 commits October 15, 2023 19:04
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
@jaekwon jaekwon mentioned this pull request Oct 15, 2023
@jaekwon
Copy link
Contributor

jaekwon commented Oct 15, 2023

interesting. so is the idea to get an "Auth" per person/account/realm/anything, and pass this around where the convention is the correct auth is expected to be passed in as context?

@moul
Copy link
Member Author

moul commented Oct 15, 2023

Yes.
The purpose is to serve as a preliminary authentication check for keys, realms, or delegations.
It also serves as a representation of an owner, storing additional metadata along with std.Address.

@jaekwon
Copy link
Contributor

jaekwon commented Oct 15, 2023

  1. This is interesting, nice start.
  2. I'm not sure if this is what is needed for Books yet, but somebody is going to want something like this.
  3. This falls under the category of "auth context passing" authentication pattern.
  4. Maybe Auth should be called AuthContext. "Auth" by itself is too general?
  5. Should it be an interface?

Regarding 5.... there is more security provided by a concrete impl rather than an interface, because the concrete impl by nature allows for more opinionated decisions. On the other hand... "auth context passing" patterns already assume that the caller knows what the caller is doing... and yet YET on the other hand, still, having concrete structures makes it less likely to program a bug.

I think this is a valid approach but I wonder if Books can do something different without the use of an Auth interface.

Seems a bit arbitrary to require Address() even though most of the time there would be an associated address... yet still doesn't feel orthogonal enough. Then, why not just a bare type HasPerm func(perm) bool ?

Kind() seems like an arbitrary distinction, something is confusing for others to implement, with no clear rule for what makes what or what to do with it. Contrast to Gno/Go "Kinds" which are precise in what they are, and there is a fixed number of them.

@jaekwon
Copy link
Contributor

jaekwon commented Oct 15, 2023

related: #1245

@jaekwon
Copy link
Contributor

jaekwon commented Oct 15, 2023

Isn't the following in some sense more "direct" than going through "permissions"?

if auth.IsAuthorized(action) { 
   // perform the action
}
// someperm comes from out of nowhere
if auth.HasPerm(someperm) {
    // perform some action independent of someperm
}

Except auth.IsAuthorized(action) doesn't make much sense because the logic for determining whether that action is not in auth but in some concrete implementation like Book.

func (book Book) Execute(action Action) error {
    // makes sure the action signatures, sequence, etc are valid.
    if err != book.Owner.Append(action); err != nil {
        return err
    }
    // the logic below could use permissions if it wanted to, or not.
    switch action.(type) {
    case: WriteAction:
    case: ReadAction:
    default: return UnrecognizedError()
    }
}

As long as it's assumed that the action will complete in one transaction, this is fine; although we are also assuming that the programmer programs such that ultimately there is a panic upon some error.

Or maybe this is desired sometimes:

func (book Book) Execute(action Action) error {
    // makes sure the action signatures, sequence, etc are valid.
    x, err := book.Owner.Reserve(action)
    if err != nil { 
        return err
    }
    // the logic below could use permissions if it wanted to, or not.
    switch action.(type) {
    case: WriteAction:
    case: ReadAction:
    default: return UnrecognizedError()
    }
    // signal that the execution reserved is complete.
    x.Done()
}

Or javascript closure like:

func (book Book) Execute(action Action) error {
    // first reserves, or panics if error
    book.Owner.Execute(action, func() {
        // if reservation was complete:
        // the logic below could use permissions if it wanted to, or not.
        switch action.(type) {
        case: WriteAction:
        case: ReadAction:
        default: return UnrecognizedError()
        }
    });
}

feels wrong though.

@moul moul changed the title feat: add p/demo/auth feat: add p/demo/authctx Oct 16, 2023
@moul
Copy link
Member Author

moul commented Oct 16, 2023

Key Updates:

  • Renamed auth to authctx and Auth to AuthContext.
  • Removed the Kind() method.
  • Replaced *Perm with a robust IsAuthorized callback system.
  • Introduced the Scope wrapper for creating privileged-dropped AuthContext instances.

moul added 2 commits October 16, 2023 15:19
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
@moul moul marked this pull request as ready for review October 16, 2023 19:13
@moul moul requested a review from a team as a code owner October 16, 2023 19:13
@jaekwon
Copy link
Contributor

jaekwon commented Oct 17, 2023

For next time, my two cents
I think it's easier to just discuss snippets of code rather than the whole implementation.
Struct and interface type defs and documentation, as if writing a basic tutorial.

}
func (a origAuthContext) Addr() std.Address { return a.addr }
func (a origAuthContext) String() string { return "orig:" + string(a.Addr()) }
func (a origAuthContext) IsAuthorized(action string) bool { return true }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can an AuthContext authorize an action without knowing from whom the action comes from?
Is this supposed to return true all the time or is it a stub?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"correct auth is expected to be passed in as context"
So I guess that makes sense, but something seems off.
An authorization should be of both the object/caller and the subject.
You can assume the caller (more on that below) but still there is no subject.
Upon what is the action authorized?
It could be a naming issue, s/action/request/g for example would work, with request.action and request.subject.

Something about this general approach feels weird though.
Here's an analogy:
In a secure facility such as a building with access controls,
you have an identity card, which you swipe onto sensors, to open doors.

  • You don't put the ACL data in the card, because this increases surface area a lot.
  • There would be all kinds of ways to attack the card itself.
  • Instead, usually the card merely holds an identity, and the ACL data is elsewhere in a central place.
  • But AuthCtx is like an access control card that has the logic of permissioning inside locally.
  • And the interfaces say that the way to use it is to implement your own access control logic card.

Wouldn't it make more sense to just pass around a Context which includes self identity, and to have a black box AuthControl.Authenticate(caller/subject, object, more action data)?

  • The user has less freedom, which is good -- less ways to mess up.
  • The security is contained within the implementation of AuthControl, easier to audit.

I don't see the benefit of the assume-the-caller AuthCtx that is exposed to the user.

Copy link
Member Author

@moul moul Oct 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, let me explain my initial idea for this package:

Think of AuthContext like JWT but simpler. It's a way to prove that something came from a trusted source and can be used for authentication. It allows you to pass around authentication information securely and preserve authenticity.

In a JWT, you have scopes or roles, but it's mainly used to manage permissions, not all the access control rules of an app.

Another analogy I like is sudo. When you're a regular user, you have limited permissions. But when you use sudo, you temporarily get more power. However, you can configure sudo to only give you specific powers for certain tasks (scopes). The final app still checks if you have the right permissions, and sudo is responsible to create the AuthContext.

AuthContext also has the advantage of being storable. Instead of just storing an address to represent an owner, you can store more context. This is useful when you want your system (like a book/boards) to be usable by different accounts, contracts, and delegates. No need for boilerplate code on your realm.

Orig/Prev AuthContexts have a default scope of "all," but when you delegate rights, you can limit them. There's also a helper to change (drop privileges) on any AuthContext, including Orig/Prev ones.

In short, AuthContext isn't just for access control lists (ACLs). Use tools like p/acl for that. For delegated contexts, scope is as important as ACLs. While ACLs verify an owner's actions, AuthContext.IsAuthorized checks the caller's rights in the current situation.

Now, I see two main directions:

  1. Keep AuthContext as it is, making it better at context management, but not a complete ACL solution. It's more of a flexible authentication context for writing apps with less boilerplate.
  2. Make AuthContext a complete ACL solution, removing the role system, and using an interface that can be implemented by different ACL mechanisms.

Possible next steps (things I plan to consider/hack on):

  • 1. Introduce authctx.IsNative(ctx) to determine where the AuthContext comes from.
  • 2. Remove ACLs, making AuthContext just a container, and add checks during object creation.
  • 3. Expand AuthContext with features like expiration.
  • 4. Allow custom IsAuthorized checks through closures.
  • 5. Improve the current p/acl library and make it interface-based.
  • 6. Rename IsAuthorized to HasScope.
  • 7. Move the ACL system in authctx to an interface implementation, so it's still there but as a default option.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to condense my general understanding here.

If it's a restrictor, then why not just fail early instead of wrapping contexts? For example, if Bob orig caller calls some realm logic (scoped to realm permissions) which calls performs some action on a Book, why not just fail early when the action can't be performed by Bob? And if Bob is allowed, then why not fail early when the realm logic is not allowed?

It seems to me this is merely delaying logic that could be performed immediately into a chain of callbacks, and what's the point of that? I don't see the benefit.

@moul moul marked this pull request as draft October 28, 2023 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: 🙏🏻 Awaiting peer review
Status: 🌟 Wanted for Launch
Development

Successfully merging this pull request may close these issues.

2 participants