Skip to content
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.

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

fix authParams showing in log with secret string(*****) #8910

Merged
merged 2 commits into from Dec 12, 2020

Conversation

hangc0276
Copy link
Contributor

Fix #8509

Changes

  1. add Secret interface and SecretsSerializer for fields need to be shown in secret string(*****) when log to json string
  2. add Secret tag for authParams and authParamMap in ClientConfiguration
  3. add a test for the secret feature

@hangc0276
Copy link
Contributor Author

@racorn Please help take a look, thanks.

@sijie sijie added this to the 2.8.0 milestone Dec 11, 2020
@sijie sijie added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Dec 11, 2020
@jiazhai jiazhai merged commit 59660e4 into apache:master Dec 12, 2020
@racorn
Copy link
Contributor

racorn commented Dec 12, 2020

@hangc0276 Thanks for you contribution. Looks good, however I think you also should include the Views class in you implementation as I suggested.

The code, as it is now, will always serialize ClientConfigurationData with '****' for the secret field. Using the Views class and using the internal view explicitly in ConfigurationDataUtils.loadData allows us to serialize ClientConfigurationData with secrets for internal use. The case to cater for here is that 1) you set auth params programmatically, and then you load a config without auth params.

Consider your test with the following modification:

    @Test
    public void testLoadSecretParams2() {
        ClientConfigurationData confData = new ClientConfigurationData();
        Map<String, String> authParamMap = new HashMap<>();
        authParamMap.put("k1", "v1");

        confData.setServiceUrl("pulsar://unknown:6650");
        confData.setAuthParams("");
        confData.setAuthParamMap(authParamMap);

        authParamMap.put("k2", "v2");

        Map<String, Object> config = new HashMap<>();
        config.put("serviceUrl", "pulsar://localhost:6650");
        config.put("authParams", "testAuthParams");
        // MODIFICATION: auth param map set on ClientConfigurationData but not on config override map
        //config.put("authParamMap", authParamMap);

        confData = ConfigurationDataUtils.loadData(config, confData, ClientConfigurationData.class);
        assertEquals("pulsar://localhost:6650", confData.getServiceUrl());
        assertEquals("testAuthParams", confData.getAuthParams());
        assertEquals("v1", confData.getAuthParamMap().get("k1"));
        assertEquals("v2", confData.getAuthParamMap().get("k2"));

        final String secretStr = "*****";
        try {
            String confDataJson = new ObjectMapper().writeValueAsString(confData);
            Map<String, Object> confDataMap = new ObjectMapper().readValue(confDataJson, Map.class);
            assertEquals("pulsar://localhost:6650", confDataMap.get("serviceUrl"));
            assertEquals(secretStr, confDataMap.get("authParams"));
            assertEquals(secretStr, confDataMap.get("authParamMap"));
        } catch (Exception e) {
            Assert.fail();
        }
    }

Then the test fails with
Caused by: com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot construct instance of java.util.LinkedHashMap(although at least one Creator exists): no String-argument constructor/factory method to deserialize from String value ('*****')

Thanks.

shoothzj added a commit that referenced this pull request Jun 5, 2022
…og (#15817)

### Motivation
See #15483 
The `@Secret` annotation works well, and introduced in #8910

### Modifications
- Revert the unneeded `@JsonIgnore`
- remove `Assert.assertFalse(s.contains("Password"));` `Password` is printed in a key. The sensitive field's value is `****`.
codelipenghui pushed a commit to codelipenghui/incubator-pulsar that referenced this pull request Jun 7, 2022
…tats log (apache#15817)

### Motivation
See apache#15483
The `@Secret` annotation works well, and introduced in apache#8910

### Modifications
- Revert the unneeded `@JsonIgnore`
- remove `Assert.assertFalse(s.contains("Password"));` `Password` is printed in a key. The sensitive field's value is `****`.

(cherry picked from commit 67361e8)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jun 7, 2022
…tats log (apache#15817)

### Motivation
See apache#15483
The `@Secret` annotation works well, and introduced in apache#8910

### Modifications
- Revert the unneeded `@JsonIgnore`
- remove `Assert.assertFalse(s.contains("Password"));` `Password` is printed in a key. The sensitive field's value is `****`.

(cherry picked from commit 67361e8)
(cherry picked from commit f8bc91f)
codelipenghui pushed a commit that referenced this pull request Jun 10, 2022
…og (#15817)

### Motivation
See #15483
The `@Secret` annotation works well, and introduced in #8910

### Modifications
- Revert the unneeded `@JsonIgnore`
- remove `Assert.assertFalse(s.contains("Password"));` `Password` is printed in a key. The sensitive field's value is `****`.

(cherry picked from commit 67361e8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/security type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Java client: cannot configure 'authParams' with PulsarClient.builder().loadConf
4 participants