[Feature/redis provider] add redis auth provider #99

SlZeroth · 2021-08-30T06:28:05Z

created provider.go file and i insert that file to all of logic about redisAuthProvider.
now createKeyProvider func is include in ServiceSet
fix config.yaml file to chose auth type

SlZeroth · 2021-08-30T07:55:04Z

I have a question about implementing the redis Provider. In the existing test code, put StaticProvider in "service.InitializeServer". However, in my code, createKeyProvider is initialized using wire DI. I'm curious what opinions you have on this and how you would like it to be changed.

SlZeroth · 2021-08-30T07:56:12Z

pkg/service/wire.go

@@ -9,14 +9,14 @@ import (
 	"github.com/livekit/livekit-server/pkg/routing"
 )

-func InitializeServer(conf *config.Config, currentNode routing.LocalNode) (*LivekitServer, error) {
+func InitializeServer(conf *config.Config, currentNode routing.LocalNode, isTest bool) (*LivekitServer, error) {


this is changed InitializeServer func

we just removed the StaticProvider code to simplify things. sorry, this ended up being a significant change. could you merge and see if issues remain?

no problem. I think remove StaticProvider version is better than before.
Deleting the StaticProvider and the CreateKeyProvider are included in the wire, so it's easier to work with.

SlZeroth · 2021-08-31T14:11:24Z

do you know about it ?

Error: The process '/opt/hostedtoolcache/mage-action/1.11.0/x64/mage' failed with exit code 1

in my local mage testall commend is work

davidzhao · 2021-09-02T05:50:10Z

cmd/server/main.go

@@ -190,7 +190,9 @@ func startServer(c *cli.Context) error {
 		return err
 	}

+


probably not worth changing this file just for spacing? :)

davidzhao · 2021-09-02T05:51:46Z

pkg/config/config.go

@@ -30,6 +30,7 @@ type Config struct {
 	KeyFile        string             `yaml:"key_file"`
 	Keys           map[string]string  `yaml:"keys"`
 	LogLevel       string             `yaml:"log_level"`
+	AuthType	   string			  `yaml:"auth_type"`


Suggested change

AuthType string `yaml:"auth_type"`

KeyProvider string `yaml:"key_provider"`

can you also run go fmt / goimports on files in this commit?

davidzhao · 2021-09-02T05:52:22Z

pkg/service/provider.go

+}
+
+func (p *RedisBasedKeyProvider) GetSecret(key string) string {
+	fmt.Print("GetSecret CALLED", "\n")


let's get rid of prints from the code

davidzhao · 2021-09-02T05:53:32Z

config-sample.yaml

@@ -60,6 +60,9 @@ keys:
  key1: secret1
  key2: secret2

+auth_type: redis


Suggested change

auth_type: redis

# # auth details could be stored in Redis. when enabled, ....

# key_provider: redis

once we decide the redis structure, let's include notes on how they keys should be stored in redis.

davidzhao · 2021-09-02T05:55:13Z

pkg/service/provider.go

+
+func (p *RedisBasedKeyProvider) GetSecret(key string) string {
+	fmt.Print("GetSecret CALLED", "\n")
+	secret, _ := p.client.Get(context.Background(), key).Result()


I could suggest storing api key/secret in a hash key. it's very hard to remove an API key without a hash of some sort. it's possible to use a hash with a name of "livekit-keys", and storing api key as key of the hash, and secret in the value.. i.e. HGET livekit-keys APIxxxxx

davidzhao · 2021-09-02T05:55:29Z

pkg/service/provider.go

+}
+
+func (p *RedisBasedKeyProvider) NumKeys() int {
+	return 1


this should return HLEN(...)

davidzhao · 2021-09-02T05:56:49Z

pkg/service/utils.go

@@ -157,3 +163,34 @@ func permissionFromGrant(claim *auth.VideoGrant) *livekit.ParticipantPermission
 	}
 	return p
 }
+
+func createKeyProvider(client *redis.Client, conf *config.Config) (auth.KeyProvider, error) {


this seems to be completely duplicated from CreateKeyProvider.

davidzhao · 2021-09-02T05:57:06Z

pkg/service/wire.go

@@ -9,6 +9,7 @@ import (
 	"github.com/livekit/livekit-server/pkg/routing"
 )

+


similarly, this file didn't need to be changed.

davidzhao · 2021-09-02T05:58:21Z

could you remove .DS_Store as well? 🙏

vonmutinda · 2021-09-03T16:49:17Z

pkg/service/utils.go

+	} else {
+		return nil, errors.New("conf.auth_type value is not current")
+	}
+}


New line here.

vonmutinda · 2021-09-03T16:53:42Z

pkg/service/wire.go

@@ -25,4 +26,4 @@ func InitializeRouter(conf *config.Config, currentNode routing.LocalNode) (routi
 	)

 	return nil, nil
-}
+}


Should we have a newline here.

thanks for your review :)

SlZeroth · 2021-09-07T07:56:38Z

fix AuthType to KeyProvider in config.go file
use hget when get api key

davidzhao

Sorry I had dropped the ball on this. This is a good time for us to re-evaluate how we are storing keys in the config. Just proposed something that is more extensible, and I'm happy to collaborate on this to finish it up too.

davidzhao · 2021-09-28T16:50:04Z

pkg/config/config.go

@@ -30,6 +30,7 @@ type Config struct {
 	KeyFile        string             `yaml:"key_file"`
 	Keys           map[string]string  `yaml:"keys"`
 	LogLevel       string             `yaml:"log_level"`
+	KeyProvider    string             `yaml:"key_provider"`


Thinking that it'd be more extensible to make this into a struct. It'll make it more flexible in the future to customize the behavior. for example:

type KeyProviderConfig struct { // env (default), config, file, redis Kind string `yaml:"kind"` // used with config and env. we could migrate the top level keys to here. Keys map[string]string // used with file provider Path string // key to use as the hash to load keys RedisKey string `yaml:"redis_key"` }

Path string what is that property mean?
because config is only one file and config file include KeyProvider information so I think Path property doesn't need .

Path can be used for the file provider.. where it could read from a separate keyfile. This is supported today, while probably not a commonly used path.

SlZeroth · 2021-11-12T06:07:46Z

Sorry i will start this task from now on
I plan to communicate slowly while reflecting the requirements.

SlZeroth · 2021-11-29T05:35:59Z

@davidzhao

The requirements for the KeyProvider structure were applied.
and I added test code for testing RedisKeyProvider.
and I applied cli option "key-provider-kind" for user choice provider

I'd appreciate it if you let me know what needs to be fixed or added.

initpwn · 2021-12-31T01:06:18Z

Any update on this PR?

CLAassistant · 2023-08-04T00:40:28Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

SlZeroth added 7 commits August 26, 2021 13:48

add: auth_type field in yaml file

01bcfb0

fix poc

7bfa8af

add: redis provider

9153499

merge with master

5d49b54

add: provider

d40dc89

pass:test

18dda84

add field : InitializeServer ( isTest )

feeccb9

SlZeroth commented Aug 30, 2021

View reviewed changes

Without StaticProvider

1096bd7

davidzhao reviewed Sep 2, 2021

View reviewed changes

vonmutinda reviewed Sep 4, 2021

View reviewed changes

SlZeroth added 4 commits September 7, 2021 16:52

use hget version and remove unnecessary code

87ab020

Merge branch 'master' into feature/redis-provider

c5c7b99

merge with master

c973baa

remove blank

2f12e1b

davidzhao reviewed Sep 28, 2021

View reviewed changes

SlZeroth added 3 commits November 12, 2021 15:01

merge with master

b17c50f

add KeyProviderConfig struct

74537e1

remove KetFile, Keys in Config struct

b4be309

SlZeroth added 6 commits November 12, 2021 15:10

change KeyProvider type to KeyProviderConfig

9dff7db

add: key-provider-kind cli option

c813529

fix: config.go for KeyProvider

7e3daa1

fix: reference KeyProvider Config

9663765

add: path properly to Config struct

254aa60

fix: createKeyProvider to support KeyProvider struct

66bc80a

add: test code to redisProvider

6391d4a

SlZeroth requested a review from davidzhao November 29, 2021 05:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature/redis provider] add redis auth provider #99

[Feature/redis provider] add redis auth provider #99

SlZeroth commented Aug 30, 2021

SlZeroth commented Aug 30, 2021

SlZeroth Aug 30, 2021

davidzhao Aug 31, 2021

SlZeroth Aug 31, 2021

SlZeroth commented Aug 31, 2021

davidzhao Sep 2, 2021

davidzhao Sep 2, 2021

davidzhao Sep 2, 2021

davidzhao Sep 2, 2021

davidzhao Sep 2, 2021

davidzhao Sep 2, 2021

davidzhao Sep 2, 2021

davidzhao Sep 2, 2021

davidzhao commented Sep 2, 2021

vonmutinda Sep 3, 2021

vonmutinda Sep 3, 2021

SlZeroth Sep 4, 2021

SlZeroth commented Sep 7, 2021

davidzhao left a comment

davidzhao Sep 28, 2021

SlZeroth Oct 18, 2021

davidzhao Oct 18, 2021

SlZeroth commented Nov 12, 2021

SlZeroth commented Nov 29, 2021 •

edited

initpwn commented Dec 31, 2021

CLAassistant commented Aug 4, 2023

		@@ -190,7 +190,9 @@ func startServer(c *cli.Context) error {
		return err
		}

	AuthType string `yaml:"auth_type"`
	KeyProvider string `yaml:"key_provider"`

	auth_type: redis
	# # auth details could be stored in Redis. when enabled, ....
	# key_provider: redis

		@@ -9,6 +9,7 @@ import (
		"github.com/livekit/livekit-server/pkg/routing"
		)

[Feature/redis provider] add redis auth provider #99

Are you sure you want to change the base?

[Feature/redis provider] add redis auth provider #99

Conversation

SlZeroth commented Aug 30, 2021

SlZeroth commented Aug 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SlZeroth commented Aug 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidzhao commented Sep 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SlZeroth commented Sep 7, 2021

davidzhao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SlZeroth commented Nov 12, 2021

SlZeroth commented Nov 29, 2021 • edited

initpwn commented Dec 31, 2021

CLAassistant commented Aug 4, 2023

SlZeroth commented Nov 29, 2021 •

edited