NIFI-15710 Always allow node identities to read connectors#10998
NIFI-15710 Always allow node identities to read connectors#10998kevdoran wants to merge 10 commits intoapache:NIFI-15258from
Conversation
|
Will review... |
| if (isRequestFromClusterNode()) { | ||
| logger.debug("Authorizing READ on Connector[{}] to cluster node [{}]", connectorId, currentUser.getIdentity()); | ||
| return; | ||
| } |
There was a problem hiding this comment.
I've verified this fix does allow the API call to proceed however, the response object isn't fully populated because permissions are evaluated again when the Entity/DTO is created and this new check is missing there.
There was a problem hiding this comment.
Thanks @mcgilman - I have updated this PR as we discussed to both solve the Entity/DTO creation issue in the service facade layer as well as make the logic for determining when requests come from node identities more reliable by using trusted http headers rather than mtls client cert identity matching against node identities.
Co-authored-by: Matt Gilman <matt.c.gilman@gmail.com>
| private boolean isClusterNodeRequest() { | ||
| if (!properties.isNode()) { | ||
| return false; | ||
| } | ||
| final String header = httpServletRequest.getHeader(RequestReplicationHeader.CLUSTER_NODE_REQUEST.getHeader()); | ||
| final boolean result = Boolean.TRUE.toString().equals(header); | ||
| logger.debug("isClusterNodeRequest: header=[{}], result={}", header, result); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
| private boolean isClusterNodeRequest() { | |
| if (!properties.isNode()) { | |
| return false; | |
| } | |
| final String header = httpServletRequest.getHeader(RequestReplicationHeader.CLUSTER_NODE_REQUEST.getHeader()); | |
| final boolean result = Boolean.TRUE.toString().equals(header); | |
| logger.debug("isClusterNodeRequest: header=[{}], result={}", header, result); | |
| return result; | |
| } | |
| private boolean isClusterNodeRequest() { | |
| if (!properties.isNode()) { | |
| return false; | |
| } | |
| if (!isRequestFromClusterNode()) { | |
| return false; | |
| } | |
| final String header = httpServletRequest.getHeader(RequestReplicationHeader.CLUSTER_NODE_REQUEST.getHeader()); | |
| final boolean result = Boolean.TRUE.toString().equals(header); | |
| logger.debug("isClusterNodeRequest: header=[{}], result={}", header, result); | |
| return result; | |
| } |
There was a problem hiding this comment.
I think we want to additionally return false here when the request is not from a cluster node. Extra defense to protect a scenario where the header is presented by a non cluster node.
…rts against known node identities
|
@mcgilman I have updated this PR based on our conversation to pick the best approach: SummaryWhen The existing implementation did exact string matching of TLS certificate DNS SANs against node API addresses, which fails with wildcard certificates (e.g. Changes:
Security Review: Wildcard SAN Matching + Proxied Entities Chain CheckWhat the code does
The four gates (all must pass)
Attack surface analysisAttack 1: Token/OIDC-authenticated user sends direct request
Attack 2: mTLS-authenticated user with their own client cert
Attack 3: User request replicated through the cluster (normal flow)
Attack 4: User spoofs
Attack 5: User spoofs both
Attack 6: Cluster node sends request with
Attack 7:
Attack 8:
Wildcard matching correctnessThe implementation: if (clientIdentity.startsWith("*.")) {
final String wildcardSuffix = clientIdentity.substring(1); // ".foo.bar"
for (final String nodeAddress : nodeApiAddresses) {
final int firstDot = nodeAddress.indexOf('.');
if (firstDot > 0 && nodeAddress.substring(firstDot).equals(wildcardSuffix)) {
Edge cases
Impact on
|
| return Objects.requireNonNull(requestReplicator, "Request Replicator required"); | ||
| } | ||
|
|
||
| private NiFiUser getNodeUser() { |
There was a problem hiding this comment.
This method is no longer used. Once removed there are a handful of imports that can be removed.
|
|
||
| final Family responseFamily = responseStatusType.getFamily(); | ||
| if (responseFamily == Family.SERVER_ERROR) { | ||
| throw new IOException("Server-side error requesting State for Connector with ID + " + connectorId + ". Status code: " + statusCode + ", reason: " + reason); |
There was a problem hiding this comment.
Connector with ID + " should probably be Connector with ID " here. Not new in this PR but wanted to note it anyways.
|
|
||
| verify(serviceFacade).authorizeAccess(any(AuthorizeAccess.class)); | ||
| verify(serviceFacade).getConnector(CONNECTOR_ID); | ||
| verify(serviceFacade).getConnector(CONNECTOR_ID, false); |
There was a problem hiding this comment.
It is possible to introduce some coverage that verifies the full flow: cluster node detected → authz bypassed → getConnector(id, true) called?
| } | ||
|
|
||
| // include the proxied entities header | ||
| // include the proxied entities header and strip untrusted headers |
There was a problem hiding this comment.
Would it be possible for some additional test coverage for when user is null and non-null?
There was a problem hiding this comment.
Also, it probably makes sense to update the javadoc for the user param to indicate it's now nullable.
@param user the user making the request, or null for internal cluster node requests (no proxy chain will be sent)
| if (clusterNodeRequest) { | ||
| logger.debug("Bypassing authorization for cluster node request on Connector [{}]", id); |
There was a problem hiding this comment.
Is this same bypass needed for the connector asset endpoints to support synchronization.
3 participants
Summary
NIFI-15710
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000VerifiedstatusPull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
./mvnw clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation