Skip to content

Commit

Permalink
fix: do not set auth-token-user, if username is given (#272)
Browse files Browse the repository at this point in the history
  • Loading branch information
jkroepke authored May 19, 2024
1 parent 9158997 commit 6460ab5
Show file tree
Hide file tree
Showing 11 changed files with 106 additions and 58 deletions.
4 changes: 2 additions & 2 deletions docs/Configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ oauth2:
validate-user: true
openvpn:
addr: "unix:///run/openvpn/server.sock"
auth-token-user: true
auth-token-user: false
auth-pending-timeout: 2m
bypass:
# common-names:
Expand Down Expand Up @@ -197,7 +197,7 @@ Usage of openvpn-auth-oauth2:
--openvpn.auth-pending-timeout duration
How long OpenVPN server wait until user is authenticated (env: CONFIG_OPENVPN_AUTH__PENDING__TIMEOUT) (default 3m0s)
--openvpn.auth-token-user
Define auth-token-user for all sessions (env: CONFIG_OPENVPN_AUTH__TOKEN__USER) (default true)
Override the username of a session with the username from the token by using auth-token-user, if the client username is empty (env: CONFIG_OPENVPN_AUTH__TOKEN__USER) (default true)
--openvpn.bypass.common-names value
bypass oauth authentication for CNs. Comma separated list. (env: CONFIG_OPENVPN_BYPASS_COMMON__NAMES)
--openvpn.common-name.environment-variable string
Expand Down
4 changes: 2 additions & 2 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ require (
github.com/madflojo/testcerts v1.1.1
github.com/stretchr/testify v1.9.0
github.com/zitadel/logging v0.6.0
github.com/zitadel/oidc/v3 v3.23.2
github.com/zitadel/oidc/v3 v3.24.0
golang.org/x/net v0.25.0
golang.org/x/oauth2 v0.20.0
golang.org/x/text v0.15.0
Expand All @@ -25,7 +25,7 @@ require (
github.com/fatih/structs v1.1.0 // indirect
github.com/fsnotify/fsnotify v1.7.0 // indirect
github.com/go-chi/chi/v5 v5.0.12 // indirect
github.com/go-jose/go-jose/v4 v4.0.1 // indirect
github.com/go-jose/go-jose/v4 v4.0.2 // indirect
github.com/go-logr/logr v1.4.1 // indirect
github.com/go-logr/stdr v1.2.2 // indirect
github.com/google/uuid v1.6.0 // indirect
Expand Down
8 changes: 4 additions & 4 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ github.com/fsnotify/fsnotify v1.7.0 h1:8JEhPFa5W2WU7YfeZzPNqzMP6Lwt7L2715Ggo0nos
github.com/fsnotify/fsnotify v1.7.0/go.mod h1:40Bi/Hjc2AVfZrqy+aj+yEI+/bRxZnMJyTJwOpGvigM=
github.com/go-chi/chi/v5 v5.0.12 h1:9euLV5sTrTNTRUU9POmDUvfxyj6LAABLUcEWO+JJb4s=
github.com/go-chi/chi/v5 v5.0.12/go.mod h1:DslCQbL2OYiznFReuXYUmQ2hGd1aDpCnlMNITLSKoi8=
github.com/go-jose/go-jose/v4 v4.0.1 h1:QVEPDE3OluqXBQZDcnNvQrInro2h0e4eqNbnZSWqS6U=
github.com/go-jose/go-jose/v4 v4.0.1/go.mod h1:WVf9LFMHh/QVrmqrOfqun0C45tMe3RoiKJMPvgWwLfY=
github.com/go-jose/go-jose/v4 v4.0.2 h1:R3l3kkBds16bO7ZFAEEcofK0MkrAJt3jlJznWZG0nvk=
github.com/go-jose/go-jose/v4 v4.0.2/go.mod h1:WVf9LFMHh/QVrmqrOfqun0C45tMe3RoiKJMPvgWwLfY=
github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A=
github.com/go-logr/logr v1.4.1 h1:pKouT5E8xu9zeFC39JXRDukb6JFQPXM5p5I91188VAQ=
github.com/go-logr/logr v1.4.1/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY=
Expand Down Expand Up @@ -71,8 +71,8 @@ github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsT
github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY=
github.com/zitadel/logging v0.6.0 h1:t5Nnt//r+m2ZhhoTmoPX+c96pbMarqJvW1Vq6xFTank=
github.com/zitadel/logging v0.6.0/go.mod h1:Y4CyAXHpl3Mig6JOszcV5Rqqsojj+3n7y2F591Mp/ow=
github.com/zitadel/oidc/v3 v3.23.2 h1:vRUM6SKudr6WR/lqxue4cvCbgR+IdEJGVBklucKKXgk=
github.com/zitadel/oidc/v3 v3.23.2/go.mod h1:9snlhm3W/GNURqxtchjL1AAuClWRZ2NTkn9sLs1WYfM=
github.com/zitadel/oidc/v3 v3.24.0 h1:TK2qUpVoX0A8Rd0Z9/1jxf+/nm5gstRKReIEG808xCI=
github.com/zitadel/oidc/v3 v3.24.0/go.mod h1:A6rYWOlTb/FtvZvUP8tl2wRCJ+wXMovfwcX80yXjMZQ=
github.com/zitadel/schema v1.3.0 h1:kQ9W9tvIwZICCKWcMvCEweXET1OcOyGEuFbHs4o5kg0=
github.com/zitadel/schema v1.3.0/go.mod h1:NptN6mkBDFvERUCvZHlvWmmME+gmZ44xzwRXwhzsbtc=
go.opentelemetry.io/otel v1.26.0 h1:LQwgL5s/1W7YiiRwxf03QGnWLb2HW4pLiAhaA5cZXBs=
Expand Down
2 changes: 1 addition & 1 deletion internal/config/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ func flagSetOpenVPN(flagSet *flag.FlagSet) {
flagSet.Bool(
"openvpn.auth-token-user",
Defaults.OpenVpn.AuthTokenUser,
"Define auth-token-user for all sessions",
"Override the username of a session with the username from the token by using auth-token-user, if the client username is empty",
)
flagSet.Duration(
"openvpn.auth-pending-timeout",
Expand Down
95 changes: 66 additions & 29 deletions internal/oauth2/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,11 @@ func TestHandler(t *testing.T) {
tests := []struct {
name string
conf config.Config
ipaddr string
state state.State
invalidState bool
xForwardedFor string
preAllow bool
postAllow bool
state string
}{
{
"default",
Expand All @@ -56,11 +56,42 @@ func TestHandler(t *testing.T) {
AuthTokenUser: true,
},
},
"127.0.0.1",
state.New(state.ClientIdentifier{CID: 0, KID: 1}, "127.0.0.1", "12345", "name"),
false,
"",
true,
true,
},
{
"with username defined",
config.Config{
HTTP: config.HTTP{
Secret: testutils.Secret,
Check: config.HTTPCheck{
IPAddr: false,
},
},
OAuth2: config.OAuth2{
Provider: "generic",
Endpoints: config.OAuth2Endpoints{},
Scopes: []string{"openid", "profile"},
Validate: config.OAuth2Validate{
Groups: make([]string, 0),
Roles: make([]string, 0),
Issuer: true,
IPAddr: false,
},
},
OpenVpn: config.OpenVpn{
Bypass: config.OpenVpnBypass{CommonNames: []string{}},
AuthTokenUser: true,
},
},
state.New(state.ClientIdentifier{CID: 0, KID: 1, UsernameIsDefined: 1}, "127.0.0.1", "12345", "name"),
false,
"",
true,
true,
"-",
},
{
"with acr values",
Expand Down Expand Up @@ -90,11 +121,11 @@ func TestHandler(t *testing.T) {
AuthTokenUser: true,
},
},
"127.0.0.1",
state.New(state.ClientIdentifier{CID: 0, KID: 1}, "127.0.0.1", "12345", "name"),
false,
"",
true,
false,
"-",
},
{
"with template",
Expand Down Expand Up @@ -122,11 +153,11 @@ func TestHandler(t *testing.T) {
AuthTokenUser: true,
},
},
"127.0.0.1",
state.New(state.ClientIdentifier{CID: 0, KID: 1}, "127.0.0.1", "12345", "name"),
false,
"",
true,
true,
"-",
},
{
"with ipaddr",
Expand All @@ -153,11 +184,11 @@ func TestHandler(t *testing.T) {
AuthTokenUser: true,
},
},
"127.0.0.1",
state.New(state.ClientIdentifier{CID: 0, KID: 1}, "127.0.0.1", "12345", "name"),
false,
"",
true,
true,
"-",
},
{
"with ipaddr + forwarded-for",
Expand Down Expand Up @@ -185,11 +216,11 @@ func TestHandler(t *testing.T) {
AuthTokenUser: true,
},
},
"127.0.0.2",
state.New(state.ClientIdentifier{CID: 0, KID: 1}, "127.0.0.2", "12345", "name"),
false,
"127.0.0.2",
true,
true,
"-",
},
{
"with ipaddr + disabled forwarded-for",
Expand Down Expand Up @@ -217,11 +248,11 @@ func TestHandler(t *testing.T) {
AuthTokenUser: true,
},
},
"127.0.0.2",
state.New(state.ClientIdentifier{CID: 0, KID: 1}, "127.0.0.2", "12345", "name"),
false,
"127.0.0.2",
false,
false,
"-",
},
{
"with ipaddr + multiple forwarded-for",
Expand Down Expand Up @@ -249,11 +280,11 @@ func TestHandler(t *testing.T) {
AuthTokenUser: true,
},
},
"127.0.0.2",
state.New(state.ClientIdentifier{CID: 0, KID: 1}, "127.0.0.2", "12345", "name"),
false,
"127.0.0.2, 8.8.8.8",
true,
true,
"-",
},
{
"with empty state",
Expand Down Expand Up @@ -281,11 +312,11 @@ func TestHandler(t *testing.T) {
AuthTokenUser: true,
},
},
"127.0.0.1",
state.State{},
false,
"127.0.0.1",
true,
true,
"",
},
{
"with invalid state",
Expand Down Expand Up @@ -313,11 +344,11 @@ func TestHandler(t *testing.T) {
AuthTokenUser: true,
},
},
"127.0.0.1",
state.State{},
true,
"127.0.0.1",
true,
true,
"test",
},
}

Expand Down Expand Up @@ -349,7 +380,7 @@ func TestHandler(t *testing.T) {

testutils.ExpectVersionAndReleaseHold(t, managementInterfaceConn, reader)

if tt.state != "-" {
if tt.state == (state.State{}) {
return
}

Expand All @@ -358,6 +389,8 @@ func TestHandler(t *testing.T) {
testutils.ExpectMessage(t, managementInterfaceConn, reader, `client-deny 0 1 "http client ip 127.0.0.1 and vpn ip 127.0.0.2 is different."`)
case !tt.postAllow:
testutils.ExpectMessage(t, managementInterfaceConn, reader, `client-deny 0 1 "client rejected"`)
case tt.state.Client.UsernameIsDefined == 1:
testutils.ExpectMessage(t, managementInterfaceConn, reader, "client-auth-nt 0 1")
default:
testutils.ExpectMessage(t, managementInterfaceConn, reader, "client-auth 0 1\r\npush \"auth-token-user aWQx\"\r\nEND")
}
Expand All @@ -384,14 +417,18 @@ func TestHandler(t *testing.T) {

time.Sleep(time.Millisecond * 100)

var err error

session := tt.state

if tt.state == "-" {
sessionState := state.New(state.ClientIdentifier{CID: 0, KID: 1}, tt.ipaddr, "12345", "name")
session, err = sessionState.Encode(tt.conf.HTTP.Secret.String())
var (
session string
err error
)

switch {
case tt.invalidState:
session = "invalid"
case tt.state == (state.State{}):
session = ""
default:
session, err = tt.state.Encode(tt.conf.HTTP.Secret.String())
if !assert.NoError(t, err) {
return
}
Expand Down Expand Up @@ -426,7 +463,7 @@ func TestHandler(t *testing.T) {

resp.Body.Close()

if tt.state != "-" {
if tt.state == (state.State{}) {
assert.Equal(t, http.StatusBadRequest, resp.StatusCode)

return
Expand Down
2 changes: 1 addition & 1 deletion internal/openvpn/callbacks.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ func (c *Client) AcceptClient(logger *slog.Logger, client state.ClientIdentifier

var err error

if c.conf.OpenVpn.AuthTokenUser {
if c.conf.OpenVpn.AuthTokenUser && client.UsernameIsDefined == 0 {
tokenUsername := base64.StdEncoding.EncodeToString([]byte(username))
_, err = c.SendCommandf("client-auth %d %d\r\npush \"auth-token-user %s\"\r\nEND", client.CID, client.KID, tokenUsername)
} else {
Expand Down
26 changes: 16 additions & 10 deletions internal/openvpn/connection/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,17 @@ import (
)

type Client struct {
KID uint64
CID uint64
Reason string
IPAddr string
IPPort string
VPNAddress string
CommonName string
SessionID string
SessionState string
IvSSO string
KID uint64
CID uint64
Reason string
IPAddr string
IPPort string
VPNAddress string
CommonName string
UsernameIsDefined int
SessionID string
SessionState string
IvSSO string
}

func NewClient(conf config.Config, message string) (Client, error) { //nolint:cyclop
Expand Down Expand Up @@ -57,12 +58,17 @@ func NewClient(conf config.Config, message string) (Client, error) { //nolint:cy
client.IPPort = envValue
case conf.OpenVpn.CommonName.EnvironmentVariableName:
client.CommonName = envValue
if conf.OpenVpn.CommonName.EnvironmentVariableName == "username" {
client.UsernameIsDefined = 1
}
case "IV_SSO":
client.IvSSO = envValue
case "session_id":
client.SessionID = envValue
case "session_state":
client.SessionState = envValue
case "username":
client.UsernameIsDefined = 1
}
case client.Reason == "" && isClientReason(line):
client.Reason, client.CID, client.KID, err = parseClientReason(line)
Expand Down
7 changes: 4 additions & 3 deletions internal/openvpn/connection/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func TestNewClientConnection(t *testing.T) {
CID: 0, KID: 1, Reason: "CONNECT", CommonName: "common_name",
IPAddr: "127.0.0.1", IPPort: "12345",
VPNAddress: "127.0.1.1", IvSSO: "webauth",
SessionID: "K3waLcCGyUuzkdXh",
SessionID: "K3waLcCGyUuzkdXh", UsernameIsDefined: 1,
},
"",
},
Expand All @@ -55,7 +55,8 @@ func TestNewClientConnection(t *testing.T) {
">CLIENT:ENV,END",
},
connection.Client{
CID: 0, KID: 1, Reason: "CONNECT", CommonName: "common_name", IPAddr: "::1", IvSSO: "webauth", SessionID: "K3waLcCGyUuzkdXh",
CID: 0, KID: 1, Reason: "CONNECT", CommonName: "common_name",
IPAddr: "::1", IvSSO: "webauth", SessionID: "K3waLcCGyUuzkdXh", UsernameIsDefined: 1,
},
"",
},
Expand All @@ -76,7 +77,7 @@ func TestNewClientConnection(t *testing.T) {
">CLIENT:ENV,END",
},
connection.Client{
CID: 0, KID: 1, Reason: "CONNECT", CommonName: "username", IPAddr: "127.0.0.1", IvSSO: "webauth",
CID: 0, KID: 1, Reason: "CONNECT", CommonName: "username", IPAddr: "127.0.0.1", IvSSO: "webauth", UsernameIsDefined: 1,
},
"",
},
Expand Down
4 changes: 4 additions & 0 deletions internal/state/state.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ type ClientIdentifier struct {
CID uint64
KID uint64
SessionID string
UsernameIsDefined int
AuthFailedReasonFile string
AuthControlFile string
}
Expand Down Expand Up @@ -64,6 +65,7 @@ func (state *State) decode(encodedState, secretKey string) error {
&state.Client.AuthFailedReasonFile,
&state.Client.AuthControlFile,
&state.Client.SessionID,
&state.Client.UsernameIsDefined,
&state.IPAddr,
&state.IPPort,
&state.CommonName,
Expand Down Expand Up @@ -105,6 +107,8 @@ func (state *State) Encode(secretKey string) (string, error) {
data.WriteString(" ")
data.WriteString(encodeString(state.Client.SessionID))
data.WriteString(" ")
data.WriteString(strconv.Itoa(state.Client.UsernameIsDefined))
data.WriteString(" ")
data.WriteString(encodeString(state.IPAddr))
data.WriteString(" ")
data.WriteString(encodeString(state.IPPort))
Expand Down
Loading

0 comments on commit 6460ab5

Please sign in to comment.