From 2e43295d63d6667901576853dd820e5271e934ac Mon Sep 17 00:00:00 2001 From: gbrodman Date: Wed, 8 Jul 2026 13:35:35 -0400 Subject: [PATCH] Sanitize a few log instances (#3135) Just remove a few possibly-insecure logs --- .../google/registry/config/RegistryConfig.java | 5 +++-- .../registry/flows/CookieSessionMetadata.java | 3 --- .../google/registry/flows/EppController.java | 14 ++++++-------- .../google/registry/flows/EppXmlSanitizer.java | 12 +++++++++--- .../java/google/registry/rde/JSchSshSession.java | 4 +--- .../google/registry/rde/RdeStagingAction.java | 3 ++- .../google/registry/rde/RdeUploadAction.java | 16 ++++++++++------ .../java/google/registry/rde/RdeUploadUrl.java | 2 +- .../spec11/GenerateSpec11ReportAction.java | 2 +- .../google/registry/rde/RdeUploadActionTest.java | 6 +++--- 10 files changed, 36 insertions(+), 31 deletions(-) diff --git a/core/src/main/java/google/registry/config/RegistryConfig.java b/core/src/main/java/google/registry/config/RegistryConfig.java index cee883033..44b3e23e3 100644 --- a/core/src/main/java/google/registry/config/RegistryConfig.java +++ b/core/src/main/java/google/registry/config/RegistryConfig.java @@ -38,6 +38,7 @@ import google.registry.dns.ReadDnsRefreshRequestsAction; import google.registry.model.common.DnsRefreshRequest; import google.registry.mosapi.MosApiClient; import google.registry.persistence.transaction.JpaTransactionManager; +import google.registry.rde.RdeUploadUrl; import google.registry.request.Action.Service; import google.registry.util.RegistryEnvironment; import google.registry.util.YamlUtils; @@ -842,8 +843,8 @@ public final class RegistryConfig { */ @Provides @Config("rdeUploadUrl") - public static URI provideRdeUploadUrl(RegistryConfigSettings config) { - return URI.create(config.rde.uploadUrl); + public static RdeUploadUrl provideRdeUploadUrl(RegistryConfigSettings config) { + return RdeUploadUrl.create(URI.create(config.rde.uploadUrl)); } /** diff --git a/core/src/main/java/google/registry/flows/CookieSessionMetadata.java b/core/src/main/java/google/registry/flows/CookieSessionMetadata.java index 1ed774ec6..9854a8eb6 100644 --- a/core/src/main/java/google/registry/flows/CookieSessionMetadata.java +++ b/core/src/main/java/google/registry/flows/CookieSessionMetadata.java @@ -19,7 +19,6 @@ import static java.nio.charset.StandardCharsets.US_ASCII; import com.google.common.base.Joiner; import com.google.common.base.Splitter; import com.google.common.collect.ImmutableSet; -import com.google.common.flogger.FluentLogger; import com.google.common.io.BaseEncoding; import google.registry.request.Response; import jakarta.servlet.http.HttpServletRequest; @@ -55,7 +54,6 @@ public class CookieSessionMetadata extends SessionMetadata { Pattern.compile("serviceExtensionUris=([^,\\s}]+)?"); private static final Pattern FAILED_LOGIN_ATTEMPTS_PATTERN = Pattern.compile("failedLoginAttempts=([^,\\s]+)?"); - private static final FluentLogger logger = FluentLogger.forEnclosingClass(); private final Map data = new HashMap<>(); @@ -66,7 +64,6 @@ public class CookieSessionMetadata extends SessionMetadata { Matcher matcher = COOKIE_PATTERN.matcher(cookie); if (matcher.find()) { String sessionInfo = decode(matcher.group(1)); - logger.atInfo().log("SESSION INFO: %s", sessionInfo); matcher = REGISTRAR_ID_PATTERN.matcher(sessionInfo); if (matcher.find()) { String registrarId = matcher.group(1); diff --git a/core/src/main/java/google/registry/flows/EppController.java b/core/src/main/java/google/registry/flows/EppController.java index 60c7a7515..d72b0eb2b 100644 --- a/core/src/main/java/google/registry/flows/EppController.java +++ b/core/src/main/java/google/registry/flows/EppController.java @@ -68,9 +68,12 @@ public final class EppController { try { eppInput = unmarshalEpp(EppInput.class, inputXmlBytes); } catch (EppException e) { - // Log the unmarshalling error, with the raw bytes (in base64) to help with debugging. + // Log the unmarshalling error, with the sanitized bytes (in base64) to help with debugging. + Optional sanitizedXml = EppXmlSanitizer.sanitizeEppXmlIfValid(inputXmlBytes); + String xmlBytesToLog = + base64().encode(sanitizedXml.map(xml -> xml.getBytes(UTF_8)).orElse(inputXmlBytes)); logger.atInfo().withCause(e).log( - "EPP request XML unmarshalling failed - \"%s\":\n%s\n%s\n%s\n%s", + "EPP request XML unmarshalling failed - \"%s\":\n%s\n%s", e.getMessage(), lazy( () -> @@ -83,12 +86,7 @@ public final class EppController { "resultMessage", e.getResult().getCode().msg, "xmlBytes", - base64().encode(inputXmlBytes)))), - LOG_SEPARATOR, - lazy( - () -> - new String(inputXmlBytes, UTF_8) - .trim()), // Charset decoding failures are swallowed. + xmlBytesToLog))), LOG_SEPARATOR); // Return early by sending an error message, with no clTRID since we couldn't unmarshal it. eppMetricBuilder.setStatus(e.getResult().getCode()); diff --git a/core/src/main/java/google/registry/flows/EppXmlSanitizer.java b/core/src/main/java/google/registry/flows/EppXmlSanitizer.java index 92cebcc4a..6c3b165f3 100644 --- a/core/src/main/java/google/registry/flows/EppXmlSanitizer.java +++ b/core/src/main/java/google/registry/flows/EppXmlSanitizer.java @@ -87,16 +87,22 @@ public class EppXmlSanitizer { * *

Also, an empty element will be formatted as {@code } instead of {@code }. */ - public static String sanitizeEppXml(byte[] inputXmlBytes) { + public static Optional sanitizeEppXmlIfValid(byte[] inputXmlBytes) { try { // Keep exactly one newline at end of sanitized string. - return CharMatcher.whitespace().trimTrailingFrom(sanitizeAndEncode(inputXmlBytes)) + "\n"; + return Optional.of( + CharMatcher.whitespace().trimTrailingFrom(sanitizeAndEncode(inputXmlBytes)) + "\n"); } catch (XMLStreamException | UnsupportedEncodingException e) { logger.atWarning().withCause(e).log("Failed to sanitize EPP XML message."); - return Base64.getMimeEncoder().encodeToString(inputXmlBytes); + return Optional.empty(); } } + public static String sanitizeEppXml(byte[] inputXmlBytes) { + return sanitizeEppXmlIfValid(inputXmlBytes) + .orElseGet(() -> Base64.getMimeEncoder().encodeToString(inputXmlBytes)); + } + private static String sanitizeAndEncode(byte[] inputXmlBytes) throws XMLStreamException, UnsupportedEncodingException { XMLEventReader xmlEventReader = diff --git a/core/src/main/java/google/registry/rde/JSchSshSession.java b/core/src/main/java/google/registry/rde/JSchSshSession.java index c29882422..aeccc864e 100644 --- a/core/src/main/java/google/registry/rde/JSchSshSession.java +++ b/core/src/main/java/google/registry/rde/JSchSshSession.java @@ -24,7 +24,6 @@ import com.jcraft.jsch.SftpException; import google.registry.config.RegistryConfig.Config; import jakarta.inject.Inject; import java.io.Closeable; -import java.net.URI; import java.time.Duration; /** @@ -57,8 +56,7 @@ final class JSchSshSession implements Closeable { * * @throws JSchException if we fail to open the connection. */ - JSchSshSession create(JSch jsch, URI uri) throws JSchException { - RdeUploadUrl url = RdeUploadUrl.create(uri); + JSchSshSession create(JSch jsch, RdeUploadUrl url) throws JSchException { logger.atInfo().log("Connecting to SSH endpoint: %s", url); Session session = jsch.getSession( url.getUser().orElse("domain-registry"), diff --git a/core/src/main/java/google/registry/rde/RdeStagingAction.java b/core/src/main/java/google/registry/rde/RdeStagingAction.java index bcd2fe2aa..bb1e5c285 100644 --- a/core/src/main/java/google/registry/rde/RdeStagingAction.java +++ b/core/src/main/java/google/registry/rde/RdeStagingAction.java @@ -312,7 +312,8 @@ public final class RdeStagingAction implements Runnable { jobRegion, new LaunchFlexTemplateRequest().setLaunchParameter(parameter)) .execute(); - logger.atInfo().log("Got response: %s", launchResponse.getJob().toPrettyString()); + logger.atInfo().log( + "Launched RDE staging Dataflow job: %s", launchResponse.getJob().getId()); jobNameBuilder.add(launchResponse.getJob().getId()); } catch (IOException e) { logger.atWarning().withCause(e).log("Pipeline Launch failed"); diff --git a/core/src/main/java/google/registry/rde/RdeUploadAction.java b/core/src/main/java/google/registry/rde/RdeUploadAction.java index e3ecf0689..c5af597e0 100644 --- a/core/src/main/java/google/registry/rde/RdeUploadAction.java +++ b/core/src/main/java/google/registry/rde/RdeUploadAction.java @@ -64,7 +64,6 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; -import java.net.URI; import java.time.Duration; import java.time.Instant; import java.util.Optional; @@ -116,11 +115,16 @@ public final class RdeUploadAction implements Runnable, EscrowTask { @Inject @Config("rdeInterval") Duration interval; @Inject @Config("rdeUploadLockTimeout") Duration timeout; @Inject @Config("rdeUploadSftpCooldown") Duration sftpCooldown; - @Inject @Config("rdeUploadUrl") URI uploadUrl; @Inject @Key("rdeReceiverKey") PGPPublicKey receiverKey; @Inject @Key("rdeSigningKey") PGPKeyPair signingKey; @Inject @Key("rdeStagingDecryptionKey") PGPPrivateKey stagingDecryptionKey; + + @Inject + @Config("rdeUploadUrl") + RdeUploadUrl rdeUploadUrl; + @Inject RdeUploadAction() {} + @Override public void run() { logger.atInfo().log("Attempting to acquire RDE upload lock for TLD '%s'.", tld); @@ -135,7 +139,7 @@ public final class RdeUploadAction implements Runnable, EscrowTask { @Override public void runWithLock(Instant watermark) throws Exception { // If a prefix is not provided,try to determine the prefix. This should only happen when the RDE - // upload cron job runs to catch up any un-retried (i. e. expected) RDE failures. + // upload cron job runs to catch up any un-retried (i.e. expected) RDE failures. String actualPrefix = prefix.orElseGet(() -> findMostRecentPrefixForWatermark(watermark, bucket, tld, gcsUtils)); logger.atInfo().log("Verifying readiness to upload the RDE deposit."); @@ -181,7 +185,7 @@ public final class RdeUploadAction implements Runnable, EscrowTask { verifyFileExists(xmlFilename); verifyFileExists(xmlLengthFilename); verifyFileExists(reportFilename); - logger.atInfo().log("Commencing RDE upload for TLD '%s' to '%s'.", tld, uploadUrl); + logger.atInfo().log("Commencing RDE upload for TLD '%s' to '%s'.", tld, rdeUploadUrl); final long xmlLength = readXmlLength(xmlLengthFilename); retrier.callWithRetry( () -> upload(xmlFilename, xmlLength, watermark, name, nameWithoutPrefix), @@ -221,10 +225,10 @@ public final class RdeUploadAction implements Runnable, EscrowTask { private void upload( BlobId xmlFile, long xmlLength, Instant watermark, String name, String nameWithoutPrefix) throws Exception { - logger.atInfo().log("Uploading XML file '%s' to remote path '%s'.", xmlFile, uploadUrl); + logger.atInfo().log("Uploading XML file '%s' to remote path '%s'.", xmlFile, rdeUploadUrl); try (InputStream gcsInput = gcsUtils.openInputStream(xmlFile); InputStream ghostrydeDecoder = Ghostryde.decoder(gcsInput, stagingDecryptionKey)) { - try (JSchSshSession session = jschSshSessionFactory.create(lazyJsch.get(), uploadUrl); + try (JSchSshSession session = jschSshSessionFactory.create(lazyJsch.get(), rdeUploadUrl); JSchSftpChannel ftpChan = session.openSftpChannel()) { ByteArrayOutputStream sigOut = new ByteArrayOutputStream(); String rydeFilename = nameWithoutPrefix + ".ryde"; diff --git a/core/src/main/java/google/registry/rde/RdeUploadUrl.java b/core/src/main/java/google/registry/rde/RdeUploadUrl.java index 83cf49ca3..368576e36 100644 --- a/core/src/main/java/google/registry/rde/RdeUploadUrl.java +++ b/core/src/main/java/google/registry/rde/RdeUploadUrl.java @@ -37,7 +37,7 @@ import javax.annotation.concurrent.Immutable; * @see RdeUploadAction */ @Immutable -final class RdeUploadUrl implements Comparable { +public final class RdeUploadUrl implements Comparable { public static final Protocol SFTP = new Protocol("sftp", 22); private static final ImmutableMap ALLOWED_PROTOCOLS = diff --git a/core/src/main/java/google/registry/reporting/spec11/GenerateSpec11ReportAction.java b/core/src/main/java/google/registry/reporting/spec11/GenerateSpec11ReportAction.java index 9ca44fc91..4b81a6fab 100644 --- a/core/src/main/java/google/registry/reporting/spec11/GenerateSpec11ReportAction.java +++ b/core/src/main/java/google/registry/reporting/spec11/GenerateSpec11ReportAction.java @@ -127,7 +127,7 @@ public class GenerateSpec11ReportAction implements Runnable { jobRegion, new LaunchFlexTemplateRequest().setLaunchParameter(parameter)) .execute(); - logger.atInfo().log("Got response: %s", launchResponse.getJob().toPrettyString()); + logger.atInfo().log("Launched Spec11 Dataflow job: %s", launchResponse.getJob().getId()); String jobId = launchResponse.getJob().getId(); if (sendEmail) { cloudTasksUtils.enqueue( diff --git a/core/src/test/java/google/registry/rde/RdeUploadActionTest.java b/core/src/test/java/google/registry/rde/RdeUploadActionTest.java index 942a61c84..4024f7b84 100644 --- a/core/src/test/java/google/registry/rde/RdeUploadActionTest.java +++ b/core/src/test/java/google/registry/rde/RdeUploadActionTest.java @@ -155,7 +155,7 @@ public class RdeUploadActionTest { action.timeout = Duration.ofSeconds(23); action.tld = "tld"; action.sftpCooldown = Duration.ofSeconds(7); - action.uploadUrl = uploadUrl; + action.rdeUploadUrl = RdeUploadUrl.create(uploadUrl); action.receiverKey = keyring.getRdeReceiverKey(); action.signingKey = keyring.getRdeSigningKey(); action.stagingDecryptionKey = keyring.getRdeStagingDecryptionKey(); @@ -212,7 +212,7 @@ public class RdeUploadActionTest { @Test void testRun() { createTld("lol"); - RdeUploadAction action = createAction(null); + RdeUploadAction action = createAction(URI.create("sftp://user:password@localhost:5432")); action.tld = "lol"; action.run(); verify(runner) @@ -231,7 +231,7 @@ public class RdeUploadActionTest { @Test void testRun_withPrefix() throws Exception { createTld("lol"); - RdeUploadAction action = createAction(null); + RdeUploadAction action = createAction(URI.create("sftp://user:password@localhost:5432")); action.prefix = Optional.of("job-name/"); action.tld = "lol"; action.run();