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.

Sign up for GitHub

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

Git Token Based Authentication #192

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Git Token Based Authentication #192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/apiserver/apiserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ func (c completedConfig) New() (*PorchServer, error) {

resolverChain := []porch.Resolver{
porch.NewBasicAuthResolver(),
porch.NewTokenAuthResolver(),
porch.NewCaBundleResolver(),
porch.NewGcloudWIResolver(coreV1Client, stsClient),
}
Expand Down
48 changes: 48 additions & 0 deletions pkg/registry/porch/secret.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package porch
import (
"context"
"fmt"
"strings"
"time"

"github.com/go-git/go-git/v5/plumbing/transport"
Expand All @@ -43,6 +44,9 @@ const (

//Secret.Data key required for the caBundle
CaBundleDataName = "ca.crt"

//Secret.Data key substring required for the token
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//Secret.Data key substring required for the token
// Secret.Data key substring required for the token

TokenDataName = "token"
)

func NewCredentialResolver(coreClient client.Reader, resolverChain []Resolver) repository.CredentialResolver {
Expand Down Expand Up @@ -143,6 +147,50 @@ func (b *BasicAuthCredential) ToAuthMethod() transport.AuthMethod {
}
}

// token based secret verification
// same as basic auth e.g. username:password
// but in token based auth username does not matter and password is the token itself
func NewTokenAuthResolver() Resolver {
return &TokenAuthResolver{}
}

var _ Resolver = &TokenAuthResolver{}

type TokenAuthResolver struct{}

type TokenAuthCredentials struct {
Username string
Password string
}

func (b *TokenAuthResolver) Resolve(_ context.Context, secret core.Secret) (repository.Credential, bool, error) {
if secret.Data[strings.ToLower(TokenDataName)] == nil && secret.Type == core.SecretTypeOpaque{
return nil, false, fmt.Errorf("Token secret.Data key must be set as %s", TokenDataName)
}

return &TokenAuthCredentials{
Username: string("Username"),
Password: string(secret.Data[TokenDataName]),
}, true, nil
}

func (b *TokenAuthCredentials) ToString() string {
return b.Password
}

var _ repository.Credential = &TokenAuthCredentials{}

func (b *TokenAuthCredentials) Valid() bool {
return true
Copy link
Member

Choose a reason for hiding this comment

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

Just checking is this OK? Is there nothing to validate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see i think this might just be a misworded function name because the basic-auth(username & password resolver & ca bundle resolver both return true for this method also) only the gcw resolver returns the following.

func (b *GcloudWICredential) Valid() bool 
{
	timeLeft := time.Until(b.token.Expiry)
	return timeLeft > 5*time.Minute
}

which to me means that check if the value is still valid based on a time interval/expiry mechanism which i dont believe apply in the other cases?
maybe a different name for the function something like expiryValidity() or something along the lines if my understanding is correct?

Copy link
Member

Choose a reason for hiding this comment

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

I think any checks we can do on validity or expiry of the token would be good.

Does the method not have to be called Valid()? I assume this must be an implementation of an interface that is being called by someone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in theory yes but this commit only allows the use of git tokens such as personal access tokens which is what we are using since were interacting with git instances for services such as github, gitlab, gitea etc and i believe these type of tokens do not actually include the expiry date in the token itself (just the key which to use) but rather manage this on the server side of the git instance itself.
As such the only way to know if the token is valid(expiry or by the value of the key itself) is to actually attempt to communicate with the git instance itself which is done after this process which simply checks that the secret data given matches what were looking for and is configured in the right way to feed it as an auth to this attempt at communicating with git

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if lets say we added JWT or perhaps other types of tokens to our list and added ability to process them which could contain other data within their token string such as the expiration date etc then another Resolver would have to be created and that Resolver Valid() method could process that information and return a more accurate validity check instead of just returning true

}

func (b *TokenAuthCredentials) ToAuthMethod() transport.AuthMethod {
return &http.BasicAuth{
Username: string(b.Username),
Password: string(b.Password),
}
}

func NewCaBundleResolver() Resolver {
return &CaBundleResolver{}
}
Expand Down
79 changes: 79 additions & 0 deletions pkg/registry/porch/secret_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"testing"
"time"

"github.com/go-git/go-git/v5/plumbing/transport/http"
"github.com/nephio-project/porch/pkg/repository"
"github.com/stretchr/testify/assert"
core "k8s.io/api/core/v1"
Expand Down Expand Up @@ -109,6 +110,84 @@ func TestCredentialResolver(t *testing.T) {
}
}

func TestTokenCredentialResolver(t *testing.T) {

testCases := map[string]struct {
readerSecret *core.Secret
readerErr error

resolverCredential repository.Credential
resolverResolved bool
resolverErr error

expectedCredential repository.Credential
expectedErrString string
}{
"secret has valid Data key": {
readerSecret: &core.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: secretName,
Namespace: secretNamespace,
},
Type: core.SecretTypeOpaque,
Data: map[string][]byte{
"token": []byte("d895131d6f99a3b65f4730e57a5989e8179be6b2"),
},
},
expectedCredential: &TokenAuthCredentials{
Username: "Username",
Password: "d895131d6f99a3b65f4730e57a5989e8179be6b2",
},
},
"secret has invalid Data key": {
readerSecret: &core.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: secretName,
Namespace: secretNamespace,
},
Type: core.SecretTypeOpaque,
Data: map[string][]byte{
"invalid": []byte("blah"),
},
},
expectedCredential: nil,
expectedErrString: "error resolving credential: Token secret.Data key must be set as token",
},
}

for tn := range testCases {
tc := testCases[tn]
t.Run(tn, func(t *testing.T) {
reader := &fakeReader{
expectedSecret: tc.readerSecret,
expectedErr: tc.readerErr,
}
credResolver := NewCredentialResolver(reader, []Resolver{
NewTokenAuthResolver(),
&fakeResolver{
credential: tc.resolverCredential,
resolved: tc.resolverResolved,
err: tc.resolverErr,
},
})

cred, err := credResolver.ResolveCredential(context.Background(), secretNamespace, secretName)
if err != nil {
assert.EqualErrorf(t, err, tc.expectedErrString, "Error should be: %v, got: %v", tc.expectedErrString, err)
}
assert.Equal(t, tc.expectedCredential, cred)
if cred != nil {
assert.Equal(t, cred.ToString(), "d895131d6f99a3b65f4730e57a5989e8179be6b2")
assert.Equal(t, cred.Valid(), true)
assert.Equal(t, cred.ToAuthMethod(), &http.BasicAuth{
Username: string("Username"),
Password: string("d895131d6f99a3b65f4730e57a5989e8179be6b2"),
})
}
})
}
}

func TestCaBundleCredentialResolver(t *testing.T) {

testCases := map[string]struct {
Expand Down
Loading