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.

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

Missing array attributes #369

Merged
merged 12 commits into from Nov 17, 2021
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -20,6 +20,7 @@ Unreleased changes are available as `avenga/couper:edge` container.
* [CORS](./docs/REFERENCE.md#cors-block) preflight requests are not blocked by access controls anymore ([#366](https://github.com/avenga/couper/pull/366))
* Reduced memory usage for backend response bodies which just get piped to the client and are not required to be read by Couper due to a variable references ([#375](https://github.com/avenga/couper/pull/375))
* However, if a huge message body is passed and additionally referenced via e.g. `json_body`, Couper may require a lot of memory for storing the data structure.
* For each SAML attribute listed in [`array_attributes`](./docs/REFERENCE.md#saml-block) at least an empty array is created in `request.context.<label>.attributes.<name>` ([#369](https://github.com/avenga/couper/pull/369))

---

Expand Down
7 changes: 6 additions & 1 deletion accesscontrol/saml2.go
Expand Up @@ -126,13 +126,18 @@ func (s *Saml2) ValidateAssertionInfo(assertionInfo *saml2.AssertionInfo) error

func (s *Saml2) GetAssertionData(assertionInfo *saml2.AssertionInfo) map[string]interface{} {
attributes := make(map[string]interface{})
// default empty slice for all arrayAttributes
for _, arrayAttrName := range s.arrayAttributes {
attributes[arrayAttrName] = []string{}
}
for _, attribute := range assertionInfo.Values {
if !contains(s.arrayAttributes, attribute.Name) {
for _, attributeValue := range attribute.Values {
attributes[attribute.Name] = attributeValue.Value
}
} else {
var attributeValues []string
// default empty slice for this arrayAttribute (instead of nil slice)
attributeValues := []string{}
malud marked this conversation as resolved.
Show resolved Hide resolved
for _, attributeValue := range attribute.Values {
attributeValues = append(attributeValues, attributeValue.Value)
}
Expand Down
104 changes: 95 additions & 9 deletions accesscontrol/saml2_test.go
Expand Up @@ -4,13 +4,13 @@ import (
"bytes"
"encoding/base64"
"encoding/xml"
"fmt"
"net/http"
"net/http/httptest"
"net/url"
"strings"
"testing"

"github.com/google/go-cmp/cmp"
saml2 "github.com/russellhaering/gosaml2"
"github.com/russellhaering/gosaml2/types"

Expand Down Expand Up @@ -150,7 +150,7 @@ func Test_SAML2ACS_GetAssertionData(t *testing.T) {
t.Fatal("Expected a saml acs object")
}

values := saml2.Values{
valuesWith2MemberOf := saml2.Values{
"displayName": types.Attribute{
Name: "displayName",
Values: []types.AttributeValue{
Expand All @@ -174,6 +174,48 @@ func Test_SAML2ACS_GetAssertionData(t *testing.T) {
},
},
}
valuesWith1MemberOf := saml2.Values{
"displayName": types.Attribute{
Name: "displayName",
Values: []types.AttributeValue{
{
Value: "Jane Doe",
},
},
},
"memberOf": types.Attribute{
Name: "memberOf",
Values: []types.AttributeValue{
{
Value: "group1",
},
},
},
}
valuesEmptyMemberOf := saml2.Values{
"displayName": types.Attribute{
Name: "displayName",
Values: []types.AttributeValue{
{
Value: "Jane Doe",
},
},
},
"memberOf": types.Attribute{
Name: "memberOf",
Values: []types.AttributeValue{},
},
}
valuesMissingMemberOf := saml2.Values{
"displayName": types.Attribute{
Name: "displayName",
Values: []types.AttributeValue{
{
Value: "Jane Doe",
},
},
},
}
var authnStatement types.AuthnStatement
err = xml.Unmarshal([]byte(`<AuthnStatement xmlns="urn:oasis:names:tc:SAML:2.0:assertion" SessionNotOnOrAfter="2020-11-13T17:06:00Z"/>`), &authnStatement)
if err != nil {
Expand All @@ -187,10 +229,10 @@ func Test_SAML2ACS_GetAssertionData(t *testing.T) {
}
for _, tc := range []testCase{
{
"without exp",
"without exp, with 2 memberOf",
&saml2.AssertionInfo{
NameID: "abc12345",
Values: values,
Values: valuesWith2MemberOf,
},
map[string]interface{}{
"sub": "abc12345",
Expand All @@ -204,15 +246,31 @@ func Test_SAML2ACS_GetAssertionData(t *testing.T) {
},
},
{
"with exp",
"without exp, with 1 memberOf",
&saml2.AssertionInfo{
NameID: "abc12345",
Values: valuesWith1MemberOf,
},
map[string]interface{}{
"sub": "abc12345",
"attributes": map[string]interface{}{
"displayName": "Jane Doe",
"memberOf": []string{
"group1",
},
},
},
},
{
"with exp, with memberOf",
&saml2.AssertionInfo{
NameID: "abc12345",
SessionNotOnOrAfter: authnStatement.SessionNotOnOrAfter,
Values: values,
Values: valuesWith2MemberOf,
},
map[string]interface{}{
"sub": "abc12345",
"exp": 1605287160,
"exp": int64(1605287160),
"attributes": map[string]interface{}{
"displayName": "Jane Doe",
"memberOf": []string{
Expand All @@ -222,11 +280,39 @@ func Test_SAML2ACS_GetAssertionData(t *testing.T) {
},
},
},
{
"without exp, empty memberOf",
&saml2.AssertionInfo{
NameID: "abc12345",
Values: valuesEmptyMemberOf,
},
map[string]interface{}{
"sub": "abc12345",
"attributes": map[string]interface{}{
"displayName": "Jane Doe",
"memberOf": []string{},
},
},
},
{
"without exp, without memberOf",
&saml2.AssertionInfo{
NameID: "abc12345",
Values: valuesMissingMemberOf,
},
map[string]interface{}{
"sub": "abc12345",
"attributes": map[string]interface{}{
"displayName": "Jane Doe",
"memberOf": []string{},
},
},
},
} {
t.Run(tc.name, func(subT *testing.T) {
assertionData := sa.GetAssertionData(tc.assertionInfo)
if fmt.Sprint(assertionData) != fmt.Sprint(tc.want) {
subT.Errorf("%s: GetAssertionData() data = %v, want %v", tc.name, assertionData, tc.want)
if !cmp.Equal(tc.want, assertionData) {
subT.Errorf(cmp.Diff(tc.want, assertionData))
}
})
}
Expand Down
2 changes: 1 addition & 1 deletion docs/REFERENCE.md
Expand Up @@ -471,7 +471,7 @@ required _label_.
| `idp_metadata_file` | string | - | File reference to the Identity Provider metadata XML file. | &#9888; required | - |
| `sp_acs_url` | string | - | The URL of the Service Provider's ACS endpoint. | &#9888; required. Relative URL references are resolved against the origin of the current request URL. The origin can be changed with the [`accept_forwarded_url`](#settings-block) attribute if Couper is running behind a proxy. | - |
| `sp_entity_id` | string | - | The Service Provider's entity ID. |&#9888; required | - |
| `array_attributes` | string | - | A list of assertion attributes that may have several values. | - | - |
| `array_attributes` | string | `[]` | A list of assertion attributes that may have several values. | Results in at least an empty array in `request.context.<label>.attributes.<name>` | `array_attributes = ["memberOf"]` |

Some information from the assertion consumed at the ACS endpoint is provided in the context at `request.context.<label>`:

Expand Down
3 changes: 0 additions & 3 deletions internal/seetie/convert.go
Expand Up @@ -85,9 +85,6 @@ func ValuesMapToValue(m url.Values) cty.Value {
}

func ListToValue(l []interface{}) cty.Value {
if len(l) == 0 {
return cty.NilVal
}
var list []cty.Value
for _, v := range l {
list = append(list, GoToValue(v))
Expand Down
21 changes: 17 additions & 4 deletions server/http_integration_test.go
Expand Up @@ -24,6 +24,7 @@ import (
"time"

"github.com/dgrijalva/jwt-go/v4"
"github.com/google/go-cmp/cmp"
"github.com/sirupsen/logrus"
logrustest "github.com/sirupsen/logrus/hooks/test"

Expand Down Expand Up @@ -3160,13 +3161,14 @@ func TestJWTAccessControl_round(t *testing.T) {
defer shutdown()

type testCase struct {
name string
path string
name string
path string
expGroups []interface{}
}

for _, tc := range []testCase{
{"separate jwt_signing_profile/jwt", "/separate"},
{"self-signed jwt", "/self-signed"},
{"separate jwt_signing_profile/jwt", "/separate", []interface{}{"g1", "g2"}},
{"self-signed jwt", "/self-signed", []interface{}{}},
} {
t.Run(tc.path, func(subT *testing.T) {
helper := test.New(subT)
Expand Down Expand Up @@ -3217,6 +3219,17 @@ func TestJWTAccessControl_round(t *testing.T) {
if pidclaim != pid {
subT.Fatalf("%q: unexpected pid claim: %q", tc.name, pidclaim)
}
groupsclaim, ok := claims["groups"]
if !ok {
subT.Fatalf("%q: missing groups claim: %#v", tc.name, claims)
}
groupsclaimArray, ok := groupsclaim.([]interface{})
if !ok {
subT.Fatalf("%q: groups must be array: %#v", tc.name, groupsclaim)
}
if !cmp.Equal(tc.expGroups, groupsclaimArray) {
subT.Errorf(cmp.Diff(tc.expGroups, groupsclaimArray))
}
})
}
}
Expand Down
3 changes: 2 additions & 1 deletion server/testdata/integration/config/08_couper.hcl
Expand Up @@ -20,7 +20,7 @@ server "jwt" {
endpoint "/{p}/create-jwt" {
response {
headers = {
x-jwt = jwt_sign("my_jwt", {})
x-jwt = jwt_sign("my_jwt", {groups = []})
}
}
}
Expand All @@ -40,6 +40,7 @@ definitions {
claims = {
iss = "the_issuer"
pid = request.path_params.p
groups = ["g1", "g2"]
}
}
jwt "my_jwt" {
Expand Down