-
Notifications
You must be signed in to change notification settings - Fork 20
feat: optional multi-authorizer #100
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
base: dev
Are you sure you want to change the base?
Conversation
pkg/identity/identity.go
Outdated
if !viper.GetBool("authorization.enabled") { | ||
return nil | ||
} | ||
|
||
// Determine required threshold: operation-specific overrides default | ||
defaultThreshold := viper.GetInt("authorization.default_threshold") | ||
opPolicy := viper.GetStringMap("authorization.operation_policies." + operation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these should be cached in the fileStore struct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implemented in 0f4bb1a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 256fdab
config.yaml.template
Outdated
operation_policies: | ||
keygen: | ||
required_authorizers: 0 | ||
authorizer_ids: [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we have authorizer_public_keys instead of _ids ? Additionally we might want support different sign algoirhtm like ed25519 and p256 so might be we need algorithm field as well.
@sivo4kin do you think we should simplify more? all operation keygen, signing, reshare should have the same authorizers and configuration to make configuration not too complex. I'm open to your ideas, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implemented in 0f4bb1a
// AuthorizeInitiatorMessage verifies that a message has sufficient valid authorizer signatures | ||
// according to the configured authorization policy. If authorization is disabled or the | ||
// required threshold resolves to zero, this is a no-op. | ||
func (s *fileStore) AuthorizeInitiatorMessage(operation string, msg types.InitiatorMessage) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think operation might not be necessary. @sivo4kin do you think we need this as apart from being used in logging, i don't see it being used anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implemented in 0f4bb1a
52b7a28
to
0f4bb1a
Compare
} | ||
|
||
// Optional multi-authorizer check for keygen | ||
if err := ec.identityStore.AuthorizeInitiatorMessage("keygen", &msg); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we may need to define these strings as constant "keygen", "signing", "reshare"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 256fdab
pkg/identity/identity.go
Outdated
// AuthorizerInfo represents a single authorizer with their public key and algorithm | ||
type AuthorizerInfo struct { | ||
PublicKey string `json:"public_key"` | ||
Algorithm string `json:"algorithm"` // "ed25519" or "secp256k1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we will support ed25519 and p256 only. P256 to be compatible with some cloud KMS
could define an enum instead of string here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 256fdab
authorizerInfo map[string]AuthorizerInfo | ||
|
||
// Cached authorization configuration | ||
authzConfig AuthorizationConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inconsistent naming convetion. authz
, authorizer
. I prefer authorizer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 256fdab
pkg/identity/identity.go
Outdated
} | ||
|
||
// Load authorization configuration | ||
store.authzConfig = AuthorizationConfig{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inconsistent naming convention
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 256fdab
pkg/identity/identity.go
Outdated
authKeys := viper.GetStringMap("authorization.authorizer_public_keys") | ||
for authID, authData := range authKeys { | ||
if authInfo, ok := authData.(map[string]interface{}); ok { | ||
info := AuthorizerInfo{ | ||
Algorithm: "ed25519", // default algorithm | ||
} | ||
|
||
if pubKey, ok := authInfo["public_key"].(string); ok && pubKey != "" { | ||
info.PublicKey = pubKey | ||
} | ||
|
||
if algo, ok := authInfo["algorithm"].(string); ok && algo != "" { | ||
info.Algorithm = algo | ||
} | ||
|
||
if info.PublicKey != "" { | ||
store.authzConfig.AuthorizerPublicKeys[authID] = info | ||
store.authorizerInfo[authID] = info | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can destruct viper configuration into a struct, defining struct can catch falsy configuration at compile time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 256fdab
pkg/identity/identity.go
Outdated
authzAuthorizers := viper.GetStringMap("authorization.authorizers") | ||
for id, v := range authzAuthorizers { | ||
// Skip if already loaded from operation-specific config | ||
if _, exists := store.authorizerInfo[id]; exists { | ||
continue | ||
} | ||
|
||
// v is expected to be a map with key "pubkey" and optional "algorithm" | ||
if entry, ok := v.(map[string]interface{}); ok { | ||
info := AuthorizerInfo{ | ||
Algorithm: "ed25519", // default algorithm | ||
} | ||
|
||
if pubHexRaw, ok := entry["pubkey"]; ok { | ||
if pubHex, ok := pubHexRaw.(string); ok && pubHex != "" { | ||
info.PublicKey = pubHex | ||
} | ||
} | ||
|
||
if algoRaw, ok := entry["algorithm"]; ok { | ||
if algo, ok := algoRaw.(string); ok && algo != "" { | ||
info.Algorithm = algo | ||
} | ||
} | ||
|
||
if info.PublicKey != "" { | ||
store.authorizerInfo[id] = info | ||
} else { | ||
logger.Warn("Invalid or empty pubkey for authorizer", "authorizerID", id) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can parse the configuration into struct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 256fdab
pkg/identity/identity.go
Outdated
// verifySignatureByAlgorithm verifies a signature using the specified algorithm | ||
func verifySignatureByAlgorithm(publicKeyHex, algorithm string, message, signature []byte) (bool, error) { | ||
switch algorithm { | ||
case "ed25519": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
define enum
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 256fdab
pkg/identity/identity.go
Outdated
} | ||
return ed25519.Verify(pubKeyBytes, message, signature), nil | ||
|
||
case "secp256k1", "p256": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only support ed25519 and p256 at the moment. We don't support secp256k1 for event signing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 256fdab
hi @sivo4kin, can you give me access to your repo my username |
…ms and backward compatibility
Adding mutil-authorised intiator