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

added extensible auth using build tags #339

Merged
merged 3 commits into from
Oct 6, 2023
Merged

Conversation

brennanjl
Copy link
Collaborator

@brennanjl brennanjl commented Oct 4, 2023

I've added a basic prototype for what it might look like for a user to implement their own auth.

Within extensions, there is a README that gives a high-level explanation. I also added one of our authentication types as an example there (ed25519 with a sha256 digest, the signature type is ed25519_nr). I originally moved all of the authentication types there, but moved it back because we rely on our "default" signers in some other tests.

I also removed the CometBFT Secp256k1 signer, since we do not use it anywhere.

@jchappelow is this along the lines of what you were thinking?

@brennanjl brennanjl marked this pull request as ready for review October 5, 2023 03:04
extensions/auth/ed25519_sha256.go Show resolved Hide resolved
pkg/auth/signer.go Outdated Show resolved Hide resolved
extensions/README.md Outdated Show resolved Hide resolved
@Yaiba
Copy link
Contributor

Yaiba commented Oct 5, 2023

All look good to me, It would be nice if we have acceptance test using specially build binaries. No necessarily to run in github workflow, just a test we can conduct locally.

@jchappelow
Copy link
Member

@jchappelow is this along the lines of what you were thinking?

Yes. To check if this could potentially reduce to a small single file addition to support an external driver implementation, I did the following:

$ go build -v -tags auth_ed25519_sha256
github.com/jchappelow/near_ed25519_sha256
github.com/kwilteam/kwil-db/extensions/auth
github.com/kwilteam/kwil-db/cmd/kwild

$ go version -m ./kwild  | grep jchappelow
	dep	github.com/jchappelow/near_ed25519_sha256	v0.0.0-20231005213826-be4f9aea73b7	h1:YcXokz9OHuyyef6+qzc0vp5JZf0txogW60eslwYlmcY=

Doing the above I think shows where there might be challenges. The two packages that would need to be imported in this approach are pkg/auth (to register) and pkg/crypto (for the standard error types we'd want to recognize from the Verify method).

These are circular module requires (not import cycles), which aren't wrong but they are annoying and probably suggest the design could change. I'm considering Go describes their sql and sql/drivers packages design:

  User Code ---> sql package (concrete types) ---> sql/driver (interfaces)

  Database Driver -> sql (to register) + sql/driver (implement interfaces)

In this approach, they have the standard library defining the interface and common concrete type sitting in between the user code (like kwild) and the driver implementation (possible 3rd party extension repo)

We're pretty close, except for the definition of the interface and the concrete types (the errors) living in pkg/auth and pkg/crypto, rather than a shared location.

@brennanjl
Copy link
Collaborator Author

Do you think we should just get rid of returning errors from the crypto package? Seems unnecessary for something that should be an example.

I like the way the package is imported into kwild, instead of being fully implemented there.

Just to make sure I understand your point regarding sql/driver: are you saying that the code that implements the defaults (secp256k1_ep and ed25519) should be in pkg/auth/drivers, and the Authenticator interface and RegisterAuthenticator() function should be the only things in pkg/auth?

@brennanjl
Copy link
Collaborator Author

And also, for the sake of this example, does that mean I should make another repo for the ed25519_sha256 signer? Or should it go elsewhere in Kwil. I'm leaning towards another repo where we can have extension examples (so that it makes it clear that they are implementable in other repos), and then doing what you do here, but curious on your thoughts.

@jchappelow
Copy link
Member

jchappelow commented Oct 5, 2023

Just to make sure I understand your point regarding sql/driver: are you saying that the code that implements the defaults (secp256k1_ep and ed25519) should be in pkg/auth/drivers, and the Authenticator interface and RegisterAuthenticator() function should be the only things in pkg/auth?

Not really, but standard implementations could go into a separate package, still in our repo. They are good for now, I think. For context, I was quoting from https://go.dev/src/database/sql/doc.txt, so interpret that diagram as you will. Mainly I was observing that there's no nice intermediate Go module like with the standard library defining the interface and having the registry. I'm not exactly sure how to solve that (if it really needs solving), but it's possible that a kwil-db/extensions repo could work, which could define both the interface and host an "example" or "canonical" extensions folder. (While closed source, we could do the register call from our end rather than their init registering with us.)

And also, for the sake of this example, does that mean I should make another repo for the ed25519_sha256 signer? Or should it go elsewhere in Kwil. I'm leaning towards another repo where we can have extension examples (so that it makes it clear that they are implementable in other repos), and then doing what you do here, but curious on your thoughts.

Potentially if that's to serve as the example.

What we're trying to accomplish is (correct me if I'm wrong) to make it so a developer can customize kwild without really modifying it's code in any meaningful way (or leaves us with only a trivial update while closed source once they've done the extension work). We pretty much achieve that by having it possible to code it in a separate repo (or at least separate Go module in our repo for the example one), with only a nameless import file in the extensions folder to activate it. They can't use our internals and the boundaries of where they should be coding are really well established.

Looking through the PR some more, but I think we're converging on a good solution. If we can get it so the extension does not need to import anything from the kwil-db main module, that's mission accomplished. Still considering though.

@brennanjl
Copy link
Collaborator Author

You're correct on what we are trying to accomplish.

Reading the database/sql doc, that makes more sense. Implementations touch the driver package, consumers of implementations (in this case, kwild) touch the concrete package.

Regarding your last point about what "mission accomplished" might look like:

  1. Is this possible, considering the user wants to make sure they are implementing the required interface?
  2. Is this ideal, since users nwould then have to register their extension by calling pkg/auth/driver.RegisterDriver() in their kwild import file, instead of in the init() function of their extension?

I see why you want to get rid of that though, the circular import is weird.

@jchappelow
Copy link
Member

  • Is this possible, considering the user wants to make sure they are implementing the required interface?

Only if the interface and any concrete type that have to be on it are defined in the hypothetical github.com/kwilteam/extensions package(s). The interface is presently pure, built-in go types only. The only thing left is we want to define some error types that extensions should return and we must recognize (pkg/crypto now)

  • Is this ideal, since users nwould then have to register their extension by calling pkg/auth/driver.RegisterDriver() in their kwild import file, instead of in the init() function of their extension?

Not really, but while closed source they have no way to do that one line of code.
The actual registry should be in our kwil-db repo with the absolutely required impls. So this is a complication.

@brennanjl
Copy link
Collaborator Author

I think it is inevitable that this system will grow into the user wanting to use more of Kwil's internals. I would imagine we continually open up more and more functionality to the extensions system; trying keep this separate will be a huge pain in the ass. Not sure if that means that we need to rethink this.

I also don't think that the error types are all too important right now, and we can probably just expect users to only use their own errors.

@jchappelow
Copy link
Member

I think it is inevitable that this system will grow into the user wanting to use more of Kwil's internals. I would imagine we continually open up more and more functionality to the extensions system; trying keep this separate will be a huge pain in the ass. Not sure if that means that we need to rethink this.

Understood. I do think we're fine with it defined in the main module of kwil-db for now.

In the not-too-distant future, the main Go Kwil SDK that would be of use to developers should be a carefully scoped Go module (or several), not the entire repo. Or so I would hope. The kwil-db repo is presently one mega module, but when things settle a little more we will absolutely want to carve out some module(s) that we anticipate third parties using most often and which don't include our development baggage or the command line apps (where these extensions are required). Any circular module requirements will vanish in the process.


As an aside, I'm continually looking at our repo from this perspective. It's helpful to consider each of our types and packages and asking if there's any reason why a third party would import it for their project, and if someone did would we be like "oh we didn't really want you using that directly" (which I see maintainers saying all the time). "types" packages with shared concrete types and interfaces are one effective ways to help achieve this instead of making an entire internal system public just for it's types (like the account store or the whole engine for instance). I always appreciate when a project defines a module that is scoped with me in mind, and it's a win for the maintainers who don't have to deal with circular module requires.

We used interfaces quite thoroughly, which is half the battle, and now we mostly just see types from very "internalish" packages traversing numerous layers of packages. Not that I think we should make one giant types package like I often see in projects, but some judicious segregation of shared type definitions from the internal machinery of the node might go a long way.

@@ -0,0 +1,58 @@
//go:build auth_ed25519_sha256 || ext_test
Copy link
Member

@jchappelow jchappelow Oct 6, 2023

Choose a reason for hiding this comment

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

@brennanjl Yeah, this was what I was trying to refer to on the call just now.

Before I moved this into https://github.com/jchappelow/near_ed25519_sha256/tree/main, I put it into a subfolder extensions/auth/near_ed25519_sha256 with a README in there explaining that extensions could also be defined externally (and that this one is in the kwil-db repo as an example). Then I left extensions/auth/near_ed25519_sha256.go as the nameless import into that package to establish the pattern (extensions defined in isolated package, just register them).

While closed source this clearly doesn't work well because they cannot compile their extension if it's is supposed to call into register. But when open source I think this would be perfectly clear to an auth extension author.

I'm not worried about the circular module requires right now. It's only pkg/auth (not counting the error types from pkg/crypto). Future module organization will smooth this out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cool, I think we can just continue with this as-is until we open source. I do think this puts more of an impetus on us to open source though.

@brennanjl
Copy link
Collaborator Author

In the not-too-distant future, the main Go Kwil SDK that would be of use to developers should be a carefully scoped Go module (or several), not the entire repo. Or so I would hope.

Agreed. I would just imagine that, over time, we will perpetually peel more and more back as it becomes:

  1. stable-r
  2. required

Another obvious example that, while not needed yet, would likely become requested, is custom SQL functions. We have the ability to very easily define our own functions, and I can see why someone might want to expand that. Things like hashing functions immediately come to mind.

It's helpful to consider each of our types and packages and asking if there's any reason why a third party would import it for their project

Yeah this is something that I don't think we have done very well so far. We import pretty liberally throughout pkg/client, because it is still internal. But it likely won't be for too long.

@brennanjl
Copy link
Collaborator Author

Wanted to loop back on this to confirm where we're at. @jchappelow are there still changes to be implemented here?

@jchappelow
Copy link
Member

Let's go ahead with it. Nothing seems outright wrong. Being close source makes it awkward, but that's temporary. This should work.

@brennanjl
Copy link
Collaborator Author

Awesome thanks. Sorry to push it, just trying to get this delivered so I can get the NEAR signer stuff off my mind and focus on other stuff

@brennanjl brennanjl merged commit 67fe11e into main Oct 6, 2023
2 checks passed
@brennanjl brennanjl deleted the compiled-extensions branch October 6, 2023 19:59
@jchappelow
Copy link
Member

jchappelow commented Oct 6, 2023

An analogy in C and C++: developing for or with an opaque component (something closed source) works with a header file that defines the interface. You either implement that interface that the component needs, or you consume that interface that the component provides. This was how I used to work with the Intel IPP library, which was closed source, but provided a header file to specify what the binary provided (theoretically I could have defined the interface). There's just much much less distinction between compilation and linking to Go developers, so the interface has to live somewhere common (or be agreed upon and implicitly satisfied).

Even in Java it's a natural thing. There was a decade-long battle between Google and Oracle over this in context of the Java APIs. Google rightly recognized that there was an API that was specified by the method signatures and that effectively formalized the interface for Java. Google replicated the declarations of certain functions in the Java API (the "headers"), but wrote its own code for the actual implementations of those functions. This allowed any Java developer to write Android apps using familiar Java API calls, but underneath those calls, Android executed Google's own code, not Oracle's (Sun Microsystems' really). They were mad, but hey it goes both directions.

Nothing we haven't talked about already, just some thoughts for more context on approaching extensions at link/compile time and why it's arguably the least friction for developers as long as the interface is public.

charithabandi pushed a commit that referenced this pull request Oct 8, 2023
* added extensible auth using build tags

* go mod tidied

* made gavins requested changes
charithabandi pushed a commit that referenced this pull request Oct 9, 2023
* added extensible auth using build tags

* go mod tidied

* made gavins requested changes
charithabandi pushed a commit that referenced this pull request Oct 9, 2023
* added extensible auth using build tags

* go mod tidied

* made gavins requested changes
charithabandi pushed a commit that referenced this pull request Oct 9, 2023
* added extensible auth using build tags

* go mod tidied

* made gavins requested changes
charithabandi pushed a commit that referenced this pull request Oct 9, 2023
* added extensible auth using build tags

* go mod tidied

* made gavins requested changes
@jchappelow jchappelow added this to the v0.6.0 milestone Nov 6, 2023
brennanjl added a commit that referenced this pull request Feb 26, 2024
* added extensible auth using build tags

* go mod tidied

* made gavins requested changes
brennanjl added a commit that referenced this pull request Feb 26, 2024
* added extensible auth using build tags

* go mod tidied

* made gavins requested changes
jchappelow pushed a commit that referenced this pull request Feb 26, 2024
* added extensible auth using build tags

* go mod tidied

* made gavins requested changes
brennanjl added a commit that referenced this pull request Feb 26, 2024
* added extensible auth using build tags

* go mod tidied

* made gavins requested changes
brennanjl added a commit that referenced this pull request Feb 26, 2024
* added extensible auth using build tags

* go mod tidied

* made gavins requested changes
brennanjl added a commit that referenced this pull request Feb 26, 2024
* added extensible auth using build tags

* go mod tidied

* made gavins requested changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants