Skip to content

fix: don't fail the whole service list when one connection can't be decrypted #31888

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.

Sign up for GitHub

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

Open
oluies wants to merge 1 commit into
base: main
Choose a base branch
from
+19 −1
Open

fix: don't fail the whole service list when one connection can't be decrypted #31888

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion ...vice/src/main/java/org/openmetadata/service/resources/services/ServiceEntityResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import jakarta.ws.rs.core.SecurityContext;
import jakarta.ws.rs.core.UriInfo;
import lombok.Getter;
import lombok.extern.slf4j.Slf4j;
import org.openmetadata.schema.ServiceConnectionEntityInterface;
import org.openmetadata.schema.ServiceEntityInterface;
import org.openmetadata.schema.entity.services.ServiceType;
Expand All @@ -37,6 +38,7 @@
import org.openmetadata.service.secrets.masker.EntityMaskerFactory;
import org.openmetadata.service.security.Authorizer;

@Slf4j
public abstract class ServiceEntityResource<
T extends ServiceEntityInterface,
R extends ServiceEntityRepository<T, S>,
Expand Down Expand Up @@ -81,7 +83,23 @@ private Object retrieveServiceConnectionConfig(T service, boolean maskPassword)

protected ResultList<T> decryptOrNullify(
SecurityContext securityContext, ResultList<T> services) {
listOrEmpty(services.getData()).forEach(service -> decryptOrNullify(securityContext, service));
// Degrade per service rather than failing the whole list. A single connection that
// cannot be decrypted -- most often after the Fernet key changes -- would otherwise
// propagate out of the loop and fail the request, hiding every other service and
// leaving no way to reach the affected one's edit form to repair it.
listOrEmpty(services.getData())
.forEach(
service -> {
try {
decryptOrNullify(securityContext, service);
} catch (Exception e) {
LOG.warn(
"Failed to decrypt connection of service '{}'; returning it without one: {}",
service.getFullyQualifiedName(),
e.getMessage());
Comment on lines +96 to +99

@greptile-apps greptile-apps Bot Aug 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 security Decryption warning exposes credentials

When field-by-field decryption fails after processing secret fields, e.getMessage() contains the generated connection object's string representation, causing password or private-key material to be written to application logs. Log the service identity without the exception message.

How this was verified: The decryption failure message embeds toDecryptObject.toString(), whose generated representation includes password fields, and this warning logs that message unchanged.

Suggested change
LOG.warn(
"Failed to decrypt connection of service '{}'; returning it without one: {}",
service.getFullyQualifiedName(),
e.getMessage());
LOG.warn(
"Failed to decrypt connection of service '{}'; returning it without one",
service.getFullyQualifiedName());

Knowledge Base Used: Auth and Security: Authentication, Authorization, Secrets, and SCIM

nullifyConnection(service);

@gitar-bot gitar-bot Bot Aug 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Quality: nullifyConnection relies on discarded return value's side effect

In the catch block, nullifyConnection(service) is called but its return value is discarded; the connection is only cleared because the generated withConnection(null) happens to mutate in place (currently true given generateBuilders=true without useInnerClassBuilders). If the codegen config ever switches to immutable/inner-class builders, withConnection would return a new instance and the original (undecryptable/encrypted) connection config would remain in the list and be serialized to the client, defeating the null-out. Assign the result back to the list to remove the hidden dependency, e.g. iterate by index and call data.set(i, nullifyConnection(service)).

Assign the nullified service back into the list instead of relying on in-place mutation.:

List<T> data = listOrEmpty(services.getData());
for (int i = 0; i < data.size(); i++) {
  T service = data.get(i);
  try {
    decryptOrNullify(securityContext, service);
  } catch (Exception e) {
    LOG.warn(
        "Failed to decrypt connection of service '{}'; returning it without one: {}",
        service.getFullyQualifiedName(),
        e.getMessage());
    data.set(i, nullifyConnection(service));
  }
}
return services;
  • Apply fix

Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎

}
});
return services;
}

Expand Down
Loading