Merge pull request #16 from virtru/updates

Misc Fixes

README.md

@@ -28,7 +28,7 @@ import (
 )
 ```
 
-See [./pdp/access-pdp-examples_test.go](./pdp/access-pdp-examples_test.go) for a complete example.
+See [./pdp/access-pdp-examples_test.go](./pdp/access-pdp-examples_test.go) for a complete example, and [Godocs here](https://pkg.go.dev/github.com/virtru/access-pdp) 
 
 ## Usage (As a gRPC server, for non-Go/remote endpoint usage)
 
@@ -53,6 +53,18 @@ GOBIN=/my-bin-dir go install github.com/virtru/access-pdp
   - Currently only the gRPC Go server code is directly used by this repo, the others are there as an example.
   - Currently we use [Buf CLI](https://buf.build/product/cli/) to lint/build/codegen from our gRPC protobuf definitions - the Buf config may be extended to generate gRPC clients and servers for any supported language. 
 
+#### gRPC server environment variables
+
+| Name | Default | Description |
+| ---- | ------- | ----------- |
+| LISTEN_PORT | "50052" | Port the gRPC server will listen on |
+| LISTEN_HOST | "localhost" | hostname the server will listen on |
+| VERBOSE | "false" | Enable verbose/debug logging |
+| DISABLE_TRACING | "false" | Disable emitting OpenTelemetry traces (avoids junk timeouts if environment has no OT collector) |
+| ENABLE_GRPC_TLS | "false" | Start gRPC server in TLS mode  |
+| GRPC_TLS_CERTFILE | "x509/server_cert.pem" | If ENABLE_GRPC_TLS is true, the certfile the server will use for TLS  |
+| GRPC_TLS_KEYFILE | "x509/server_key.pem" | If ENABLE_GRPC_TLS is true, the keyfile the server will use for TLS  |
+
 ## Design Details
 In this implementation, the Access PDP:
 
@@ -89,3 +101,4 @@ For each entity identifer provided:
 ### Interface
 
 This library exposes gRPC endpoints, and so can be consumed by any code that understands the gRPC protocol. This library could be wrapped in a container and hosted out-of-process from an Access PEP, or it could be hosted in-process.
+

pdp/access-pdp.go

@@ -203,7 +203,7 @@ func (pdp *AccessPDP) allOfRule(dataAttrsBySingleCanonicalName []attrs.Attribute
 	ruleResultsByEntity := make(map[string]DataRuleResult)
 
 	//All of the data AttributeInstances in the arg have the same canonical name.
-	pdp.logger.Debug("Evaluating all-of decision for data attr %s", dataAttrsBySingleCanonicalName[0].GetCanonicalName())
+	pdp.logger.Debugf("Evaluating all-of decision for data attr %s", dataAttrsBySingleCanonicalName[0].GetCanonicalName())
 
 	//Go through every entity's AttributeInstance set...
 	for entityId, entityAttrs := range entityAttributes {
@@ -216,7 +216,7 @@ func (pdp *AccessPDP) allOfRule(dataAttrsBySingleCanonicalName []attrs.Attribute
 		//For every unqiue data AttributeInstance (that is, unique data attribute value) in this set of data AttributeInstances sharing the same canonical name...
 		for _, dataAttrVal := range dataAttrsBySingleCanonicalName {
 			dvCanonicalName := dataAttrVal.GetCanonicalName()
-			pdp.logger.Debug("Evaluating all-of decision for data attr %s with value %s", dvCanonicalName, dataAttrVal.Value)
+			pdp.logger.Debugf("Evaluating all-of decision for data attr %s with value %s", dvCanonicalName, dataAttrVal.Value)
 			//See if
 			// 1. there exists an entity AttributeInstance in the set of AttributeInstances
 			// with the same canonical name as the data AttributeInstance in question
@@ -285,8 +285,8 @@ func (pdp *AccessPDP) anyOfRule(dataAttrsBySingleCanonicalName []attrs.Attribute
 			//If we did not find the data AttributeInstance canonical name + value in the entity AttributeInstance set,
 			//then prepare a ValueFailure for that data AttributeInstance and value, for this entity
 			if !found {
-				denialMsg = fmt.Sprintf("anyOf not satisfied for canonical data attr+value %s and entity %s", dataAttrVal, entityId)
-				pdp.logger.Warn(denialMsg)
+				denialMsg = fmt.Sprintf("anyOf not satisfied for canonical data attr+value %s and entity %s - anyOf is permissive, so this doesn't mean overall failure", dataAttrVal, entityId)
+				pdp.logger.Debug(denialMsg)
 
 				valueFailures = append(valueFailures, ValueFailure{
 					DataAttribute: &dataAttrVal,
@@ -325,11 +325,13 @@ func (pdp *AccessPDP) hierarchyRule(dataAttrsBySingleCanonicalName []attrs.Attri
 	ruleResultsByEntity := make(map[string]DataRuleResult)
 
 	highestDataInstance := pdp.getHighestRankedInstanceFromDataAttributes(order, dataAttrsBySingleCanonicalName)
-	dvCanonicalName := highestDataInstance.GetCanonicalName()
+	if highestDataInstance == nil {
+		pdp.logger.Warnf("No data attribute value found that matches attribute definition allowed values! All entity access will be rejected!")
+	} else {
+		pdp.logger.Debugf("Highest ranked hierarchy value on data attributes is: %s", highestDataInstance)
+	}
 	//All of the data AttributeInstances in the arg have the same canonical name.
-	pdp.logger.Debugf("Evaluating hierarchy decision for data attr %s", dvCanonicalName)
 
-	pdp.logger.Debugf("Highest ranked hierarchy value on data attributes is: %s", highestDataInstance)
 
 	//Go through every entity's AttributeInstance set...
 	for entityId, entityAttrs := range entityAttributes {
@@ -339,22 +341,35 @@ func (pdp *AccessPDP) hierarchyRule(dataAttrsBySingleCanonicalName []attrs.Attri
 		//Cluster entity AttributeInstances by canonical name...
 		entityAttrCluster := attrs.ClusterByCanonicalName(entityAttrs)
 
-		//For every unique data AttributeInstance (that is, value) in this set of data AttributeInstances sharing the same canonical name...
-		pdp.logger.Debugf("Evaluating hierarchy decision for data attr %s with value %s", dvCanonicalName, highestDataInstance.Value)
+		if highestDataInstance != nil {
+			dvCanonicalName := highestDataInstance.GetCanonicalName()
+			//For every unique data AttributeInstance (that is, value) in this set of data AttributeInstances sharing the same canonical name...
+			pdp.logger.Debugf("Evaluating hierarchy decision for data attr %s with value %s", dvCanonicalName, highestDataInstance.Value)
 
-		//Compare the (one or more) AttributeInstances (that is, values) for this canonical name to the (one) data AttributeInstance, and see which is "higher".
-		entityPassed = entityRankGreaterThanOrEqualToDataRank(order, highestDataInstance, entityAttrCluster[dvCanonicalName])
-		denialMsg := ""
+			//Compare the (one or more) AttributeInstances (that is, values) for this canonical name to the (one) data AttributeInstance, and see which is "higher".
+			entityPassed = entityRankGreaterThanOrEqualToDataRank(order, highestDataInstance, entityAttrCluster[dvCanonicalName])
 
-		//If the rank of the data AttributeInstance (that is, value) is higher than the highest entity AttributeInstance, then FAIL.
-		if !entityPassed {
-			denialMsg = fmt.Sprintf("Hierarchy - Entity: %s hierarchy values rank below data hierarchy value of %s", entityId, highestDataInstance.Value)
-			pdp.logger.Warn(denialMsg)
+			//If the rank of the data AttributeInstance (that is, value) is higher than the highest entity AttributeInstance, then FAIL.
+			if !entityPassed {
+				denialMsg := fmt.Sprintf("Hierarchy - Entity: %s hierarchy values rank below data hierarchy value of %s", entityId, highestDataInstance.Value)
+				pdp.logger.Warn(denialMsg)
 
-			//Since there is only one data value we (ultimately) consider in a HierarchyRule, we will only ever
-			//have one ValueFailure per entity at most
+				//Since there is only one data value we (ultimately) consider in a HierarchyRule, we will only ever
+				//have one ValueFailure per entity at most
+				valueFailures = append(valueFailures, ValueFailure{
+					DataAttribute: highestDataInstance,
+					Message:       denialMsg,
+				})
+			}
+			//It's possible we couldn't FIND a highest data value - because none of the data values are in the set of valid attribute definition values!
+			//If this happens, we can't do a comparison, and access will be denied for every entity for this data attribute instance
+		} else {
+			//If every data attribute value we're comparing against is invalid (that is, none of them exist in the attribute definition)
+			//then we must fail and return a nil instance.
+			denialMsg := fmt.Sprintf("Hierarchy - No data values found exist in attribute definition, no hierarchy comparison possible, entity %s is denied", entityId)
+			pdp.logger.Warn(denialMsg)
 			valueFailures = append(valueFailures, ValueFailure{
-				DataAttribute: highestDataInstance,
+				DataAttribute: nil,
 				Message:       denialMsg,
 			})
 		}
@@ -398,24 +413,35 @@ func (pdp *AccessPDP) groupByFilterEntityAttributeInstances(entityAttributes map
 //Since by definition hierarchy comparisons have to be one-data-value-to-many-entity-values, this won't work.
 //So, in a scenario where there are multiple data values to choose from, grab the "highest" ranked value
 //present in the set of data AttributeInstances, and use that as the point of comparison, ignoring the "lower-ranked" data values.
+//If we find a data value that does not exist in the attribute definition's list of valid values, we will skip it
+//If NONE of the data values exist in the attribute defintiions list of valid values, return a nil instance
 func (pdp *AccessPDP) getHighestRankedInstanceFromDataAttributes(order []string, dataAttributeCluster []attrs.AttributeInstance) *attrs.AttributeInstance {
 	//For hierarchy, convention is 0 == most privileged, 1 == less privileged, etc
 	//So initialize with the LEAST privileged rank in the defined order
 	var highestDVIndex int = (len(order) - 1)
-	var highestRankedInstance attrs.AttributeInstance
+	var highestRankedInstance *attrs.AttributeInstance = nil
 	for _, dataAttr := range dataAttributeCluster {
 		foundRank := getOrderOfValue(order, dataAttr.Value)
+		if foundRank == -1 {
+			msg := fmt.Sprintf("Data value %s is not in %s and is not a valid value for this attribute - ignoring this invalid value and continuing to look for a valid one...", dataAttr.Value, order)
+			pdp.logger.Warn(msg)
+			//If this isnt a valid data value, skip this iteration and look at the next one - maybe it is?
+			//If none of them are valid, we should return a nil instance
+			continue
+		}
 		pdp.logger.Debugf("Found data rank %d for value %s", foundRank, dataAttr.Value)
 		pdp.logger.Debugf("current max rank is %d", highestDVIndex)
 		//If this rank is a "higher rank" (that is, a lower index) than the last one,
+		//(or it is the same rank, to handle cases where the lowest is the only)
 		//it becomes the new high water mark rank.
-		if foundRank < highestDVIndex {
+		if foundRank <= highestDVIndex {
 			pdp.logger.Debugf("Updating rank!\n")
 			highestDVIndex = foundRank
-			highestRankedInstance = dataAttr
+			gotAttr := dataAttr
+			highestRankedInstance = &gotAttr
 		}
 	}
-	return &highestRankedInstance
+	return highestRankedInstance
 }
 
 //Given a single AttributeInstance, and an arbitrary set of AttributeInstances,
@@ -475,12 +501,13 @@ func entityRankGreaterThanOrEqualToDataRank(order []string, dataAttribute *attrs
 //return the rank #/index of the singular AttributeInstance
 func getOrderOfValue(order []string, value string) int {
 	//For hierarchy, convention is 0 == most privileged, 1 == less privileged, etc
-	//So initialize with the LEAST privileged rank in the defined order
-	var dvIndex int = (len(order) - 1)
+	dvIndex := -1 // -1 == Not Found in the set - this should always be a failure.
 	for index := range order {
 		if order[index] == value {
 			dvIndex = index
 		}
 	}
+
+	//We either found the right index, or we return -1
 	return dvIndex
 }

pdp/access-pdp_test.go

@@ -919,6 +919,48 @@ func Test_AccessPDP_Hierarchy_FailEntityValueTooLow(t *testing.T) {
 	assert.Equal(t, &mockAttrDefinitions[0], decisions[entityID].Results[0].RuleDefinition)
 }
 
+func Test_AccessPDP_Hierarchy_FailEntityValueAndDataValuesBothLowest(t *testing.T) {
+	zapLog, _ := zap.NewDevelopment()
+
+	entityID := "4f6636ca-c60c-40d1-9f3f-015086303f74"
+	attrAuthorities := []string{"https://example.org"}
+	mockAttrDefinitions := []attrs.AttributeDefinition{
+		{
+			Authority: attrAuthorities[0],
+			Name:      "MyAttr",
+			Rule:      "hierarchy",
+			Order:     []string{"Privileged", "LessPrivileged", "NotPrivilegedAtAll"},
+			// GroupBy *AttributeInstance `json:"group_by,omitempty"`
+		},
+	}
+	mockDataAttrs := []attrs.AttributeInstance{
+		{
+			Authority: attrAuthorities[0],
+			Name:      mockAttrDefinitions[0].Name,
+			Value:     mockAttrDefinitions[0].Order[2],
+		},
+	}
+	mockEntityAttrs := map[string][]attrs.AttributeInstance{
+		entityID: {
+			{
+				Authority: "https://example.org",
+				Name:      "MyAttr",
+				Value:     "NotPrivilegedAtAll",
+			},
+		},
+	}
+	accessPDP := NewAccessPDP(zapLog.Sugar())
+
+	decisions, err := accessPDP.DetermineAccess(mockDataAttrs, mockEntityAttrs, mockAttrDefinitions, ctx.Background())
+
+	assert.Nil(t, err)
+	assert.True(t, decisions[entityID].Access)
+	assert.Equal(t, 1, len(decisions[entityID].Results))
+	assert.True(t, decisions[entityID].Results[0].Passed)
+	assert.Equal(t, 0, len(decisions[entityID].Results[0].ValueFailures))
+	assert.Equal(t, &mockAttrDefinitions[0], decisions[entityID].Results[0].RuleDefinition)
+}
+
 func Test_AccessPDP_Hierarchy_FailEntityValueOrder(t *testing.T) {
 	zapLog, _ := zap.NewDevelopment()
 
@@ -1130,6 +1172,7 @@ func Test_AccessPDP_Hierarchy_FailDataValueNotInOrder(t *testing.T) {
 	assert.False(t, decisions[entityID].Results[0].Passed)
 	assert.Equal(t, 1, len(decisions[entityID].Results[0].ValueFailures))
 	assert.Equal(t, &mockAttrDefinitions[0], decisions[entityID].Results[0].RuleDefinition)
+	assert.Nil(t, decisions[entityID].Results[0].ValueFailures[0].DataAttribute)
 }
 
 func Test_AccessPDP_Hierarchy_PassWithMixedKnownAndUnknownDataOrder(t *testing.T) {