28Hus commented on code in PR #1565:
URL: https://github.com/apache/dubbo-admin/pull/1565#discussion_r4110054787
##########
pkg/config/console/auth/config.go:
##########
@@ -89,8 +91,11 @@ func (c *Config) Validate() error {
if c.ExpirationTime <= 0 || c.ExpirationTime >= 24*60*60 {
return errors.New("auth: expirationTime should be greater than
0 and less than 86400")
}
- if c.SessionSecret == "" {
- c.SessionSecret = DefaultSessionSecret
+ if strings.TrimSpace(c.SessionSecret) == "" {
+ c.SessionSecret = os.Getenv(SessionSecretEnvVar)
+ }
+ if strings.TrimSpace(c.SessionSecret) == "" ||
len([]byte(c.SessionSecret)) < MinimumSessionSecretLength {
+ return fmt.Errorf("auth: sessionSecret must contain at least %d
bytes", MinimumSessionSecretLength)
Review Comment:
Thanks for flagging this. However, the claimed authentication bypass is a
false positive. In the reviewed revision, config.Load() invokes
AdminConfig.PreProcess() before Validate(). With console: null,
c.Console.PreProcess() panics on the nil pointer, so execution never reaches
cookie.NewStore. In addition, Gorilla securecookie treats an empty hash key as
unset and rejects cookie encoding and decoding; it cannot be used to forge a
session.
I have since changed this path to fail closed explicitly: missing or null
console configuration is rejected during preprocessing, and an empty console
object is rejected during validation. Regression tests cover these cases,
including when a valid session-secret environment variable is set.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
