Skip to content

Conversation

@paulo-ocean
Copy link
Contributor

@paulo-ocean paulo-ocean commented Feb 26, 2025

Fixes #810 #804

Changes proposed in this PR:

  • change creds behaviour for type 'address'
  • refactor + some aux functions
  • new Policy server specific type

@paulo-ocean paulo-ocean self-assigned this Feb 27, 2025
@paulo-ocean paulo-ocean marked this pull request as ready for review March 3, 2025 11:35
@paulo-ocean paulo-ocean marked this pull request as draft March 7, 2025 09:37
@paulo-ocean paulo-ocean marked this pull request as ready for review March 10, 2025 11:55
Comment on lines 86 to 89
// if no address-based credentials are defined (both allow and deny lists are empty), access to the asset is restricted to everybody;
// to allow access to everybody, the symbol * will be used in the allow list;
// if a web3 address is present on both deny and allow lists, the deny list takes precedence
// and access to the asset is denied for the respective address.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm kind of surprised that we agreed to implement this change. So the default behaviour is that if an asset published, no one can access it? Seems contrary to our mission statement 🤔

Copy link
Contributor Author

@paulo-ocean paulo-ocean Mar 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's copied from the specs yes #804 #810 OceanProtocolEnterprise#25
makes perfectly sense to me for security reasons.

Copy link
Contributor

@jamiehewitt15 jamiehewitt15 Mar 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense to enterprise users... In the past Ocean has always taken the approach that everything is open by default and Enterprises must take extra steps if they don't like the default. So this is a fundamental shift in that approach

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not gonna discuss that... please check with @alexcos20 . I just followed the specs and they make sense to me

Copy link
Contributor

@jamiehewitt15 jamiehewitt15 Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see anything changing if we have a discussion, you may as well just go ahead. Nothing wrong with the implementation

Copy link
Contributor

@jamiehewitt15 jamiehewitt15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

paulo-ocean and others added 3 commits March 14, 2025 08:38
…-accessList

add support for accessList on download checkCredentials
# Conflicts:
#	src/@types/DDO/Credentials.ts
#	src/test/unit/credentials.test.ts
#	src/utils/credentials.ts
@giurgiur99 giurgiur99 self-requested a review as a code owner April 22, 2025 07:29
@giurgiur99 giurgiur99 assigned giurgiur99 and unassigned paulo-ocean Apr 23, 2025
Copy link
Contributor

@mariacarmina mariacarmina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

# Conflicts:
#	package-lock.json
#	package.json
@alexcos20 alexcos20 assigned alexcos20 and unassigned giurgiur99 Jan 6, 2026
Copy link
Member

@alexcos20 alexcos20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI automated code review (Gemini 3).

Overall risk: medium

Summary:
This pull request refactors and enhances the credential checking mechanism, primarily driven by an update to the @oceanprotocol/ddo-js library (from 0.0.6 to 0.0.8). The core checkCredentials function has been made asynchronous, now supports accessList type credentials by interacting with blockchain contracts, and introduces match_allow and match_deny rules (supporting 'any' and 'all' logic). Crucially, the default behavior for undefined or empty credentials has shifted from implicitly allowing access to explicitly denying it, which is a significant security improvement. Unit tests have been updated and extended to cover these new behaviors and asynchronous calls.

Comments:
• [WARNING][security] The current logic accessGrantedDDOLevel = areKnownCredentialTypes(ddo.credentials) ? await checkCredentials(...) : true implies that if the DDO contains unknown credential types (areKnownCredentialTypes returns false), access is automatically granted. This could be a security vulnerability if an attacker uses an unknown credential type to bypass checks. It might be safer to default to false (deny access) if unknown credential types are encountered, or to log a severe warning.

Consider changing true to false here, or handling unknown types explicitly with a security policy.
• [WARNING][security] Similar to the DDO-level credentials, the logic accessGrantedServiceLevel = areKnownCredentialTypes(service.credentials) ? await checkCredentials(...) : true could lead to a security bypass if service-level credentials contain unknown types. If areKnownCredentialTypes returns false, access is currently granted.

Consider changing true to false here to deny access by default for unknown credential types at the service level.
• [INFO][style] The findCredential function (which was removed) used to map values to lowercase. The new isAddressCredentialMatch function also maps to lowercase. Ensure this case-insensitivity is consistently applied where needed for address comparisons to prevent unexpected mismatches.
• [INFO][other] The checkAccessListCredential function handles chainId not being provided or supportedNetwork not found by returning false. This is good fail-safe behavior. However, it's worth noting that if a DDO specifies an accessList credential but no chainId is provided in the context of the check, this credential type will effectively be ignored (denied).

This behavior is acceptable, but it should be clearly documented for users creating DDOs with accessList credentials.
• [INFO][test] The unit tests cover the new match_allow: any and match_allow: all logic, as well as the default deny behavior. This is excellent. However, there aren't specific tests that mock and verify the behavior of checkAccessListCredential interacting with a simulated AccessList contract. This would involve mocking the getBlockchainHandler, getSigner, and getNFTContract calls, and then simulating findAccountFromAccessList responses.

While the try-catch in findAccessListCredentials provides safety, explicit tests for both successful and failed access list lookups would increase confidence in this critical path.
• [INFO][dependency] The @oceanprotocol/ddo-js package has been updated from 0.0.6 to 0.0.8. This update involves changes in its direct dependencies (e.g., removal of crypto, jose, @rdfjs/dataset and addition of rdf-data-factory, rdf-literal). Given the impact on credential logic, it's a significant change. Ensure that the new ddo-js version is fully compatible and any breaking changes or updated assumptions are handled.

Copy link
Member

@alexcos20 alexcos20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI automated code review (Gemini 3).

Overall risk: medium

Summary:
This pull request significantly refactors the credential checking mechanism within Ocean Node. It upgrades the @oceanprotocol/ddo-js dependency from 0.0.6 to 0.0.8, centralizing credential type definitions. The checkCredentials function is made asynchronous to support on-chain checks for accessList credentials and its internal logic has been rewritten to explicitly handle allow and deny lists with match_allow and match_deny rules. Notably, the default behavior for credential validation has changed: if no explicit allow or deny rules match, access is now denied by default, contrasting with the previous implicit allowance. New helper functions (isValidCredentialsList, isAddressCredentialMatch, checkAddressCredential, checkAccessListCredential, checkDenyList, checkAllowList, isAddressMatchAll) have been introduced to modularize the logic, and a utility function getBlockchainHandler has been added for blockchain interactions.

Comments:
• [ERROR][bug] The default behavior of checkCredentials has changed from implicitly allowing access (returning true for undefined or empty credentials/lists) to explicitly denying access (returning false). This is a breaking change that impacts the security posture and could unintentionally restrict access for existing assets or configurations that relied on the previous lenient default. Please confirm this change is intentional and consider explicit migration paths or clear documentation for users.
• [ERROR][security] The checkDenyList function currently only evaluates CREDENTIALS_TYPES.ADDRESS rules. It does not include logic to check CREDENTIALS_TYPES.ACCESS_LIST rules for denial. If an accessList is specified in the deny array, users belonging to that access list would still be granted access, which is a potential security vulnerability. Please extend the checkDenyList to correctly evaluate accessList credentials.
• [WARNING][bug] In checkAddressCredential, if an address matches an ADDRESS type rule or a wildcard * rule, it immediately returns true if match_allow is 'any' (or undefined). This logic effectively short-circuits the match_allow: 'all' condition for the ADDRESS credential type, potentially allowing access even if other required credential types in the allow list (e.g., ACCESS_LIST) are not met when match_allow: 'all' is specified. Please clarify if match_allow: 'all' applies across all credential types in the allow list, or only within a specific credential type. If it applies across all types, the short-circuit for ADDRESS needs to be reconsidered.
• [INFO][other] The @oceanprotocol/ddo-js dependency has been updated, and package-lock.json shows significant changes in its transitive dependencies, including the removal of crypto, jose, lodash, and axios. While this might be an internal optimization within ddo-js, it's worth noting these changes as they might have implications for bundle size, security audits, or potential conflicts if other parts of ocean-node also directly used older versions of these libraries for different purposes. Please ensure that these dependency changes do not negatively impact other functionalities or introduce new vulnerabilities.
• [INFO][style] The new getBlockchainHandler utility function is a good addition for reducing boilerplate code. Consider adding a JSDoc comment for its purpose, parameters, and return value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Credentials address: Change default behaviour

6 participants