Skip to content

[security] Remove sensitive msg from consumer/producer stats log - #15483

Merged
hezhangjian merged 1 commit into
apache:masterfrom
hezhangjian:password-print
May 8, 2022
Merged

[security] Remove sensitive msg from consumer/producer stats log#15483
hezhangjian merged 1 commit into
apache:masterfrom
hezhangjian:password-print

Conversation

@hezhangjian

Copy link
Copy Markdown
Member

Motivation

Currently, we are print password field to consumer/producer stats log

Modification

  • add missed @JsonIgnore on field and getMethod
  • delete unused withoutAttribute call

@hezhangjian hezhangjian added the doc-not-needed Your PR changes do not impact docs label May 7, 2022
@hezhangjian hezhangjian self-assigned this May 7, 2022
@hezhangjian
hezhangjian merged commit 8b2f3dd into apache:master May 8, 2022
@hezhangjian
hezhangjian deleted the password-print branch May 8, 2022 23:39
codelipenghui pushed a commit that referenced this pull request May 20, 2022
)

Currently, we are print password field to consumer/producer stats log

- add missed `@JsonIgnore` on field and getMethod
- delete unused `withoutAttribute` call

(cherry picked from commit 8b2f3dd)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request May 23, 2022
…che#15483)

Currently, we are print password field to consumer/producer stats log

- add missed `@JsonIgnore` on field and getMethod
- delete unused `withoutAttribute` call

(cherry picked from commit 8b2f3dd)
(cherry picked from commit 6cbead5)
mattisonchao pushed a commit that referenced this pull request May 25, 2022
)

### Motivation
Currently, we are print password field to consumer/producer stats log

### Modification
- add missed `@JsonIgnore` on field and getMethod
- delete unused `withoutAttribute` call

(cherry picked from commit 8b2f3dd)
@mattisonchao mattisonchao added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label May 25, 2022

@michaeljmarshall michaeljmarshall left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The @Secret annotation was supposed to ensure these are serialized as *** when present. Do we know why the SecretsSerializer wasn't working here?

@hezhangjian

Copy link
Copy Markdown
Member Author

@michaeljmarshall The @Secret is not recognized by jackson writer.

@michaeljmarshall

michaeljmarshall commented May 27, 2022

Copy link
Copy Markdown
Member

@michaeljmarshall The @Secret is not recognized by jackson writer.

@shoothzj - I don't believe that is correct. When I remove the @JsonIgnore annotations that you added, and run your test while logging the output, here is what I get:

{"serviceUrl":null,"authPluginClassName":null,"authParams":null,"authParamMap":null,"operationTimeoutMs":30000,"lookupTimeoutMs":30000,"statsIntervalSeconds":60,"numIoThreads":1,"numListenerThreads":1,"connectionsPerBroker":1,"useTcpNoDelay":true,"useTls":false,"tlsTrustCertsFilePath":"","tlsAllowInsecureConnection":false,"tlsHostnameVerificationEnable":true,"concurrentLookupRequest":5000,"maxLookupRequest":50000,"maxLookupRedirects":20,"maxNumberOfRejectedRequestPerConnection":50,"keepAliveIntervalSeconds":30,"connectionTimeoutMs":10000,"requestTimeoutMs":60000,"initialBackoffIntervalNanos":100000000,"maxBackoffIntervalNanos":60000000000,"enableBusyWait":false,"listenerName":null,"useKeyStoreTls":false,"sslProvider":null,"tlsTrustStoreType":"JKS","tlsTrustStorePath":null,"tlsTrustStorePassword":"*****","tlsCiphers":[],"tlsProtocols":[],"memoryLimitBytes":67108864,"proxyServiceUrl":null,"proxyProtocol":null,"enableTransaction":false,"dnsLookupBindAddress":null,"dnsLookupBindPort":0,"socks5ProxyAddress":null,"socks5ProxyUsername":null,"socks5ProxyPassword":"*****"}

Note that the @Secret fields correctly have **** in the names.

The @Secret annotation was added here, #8910, and the annotation's interface definitely shows an integration with Jackson. That being said, perhaps you found a case where the Jackson serializer wasn't interpreting the annotation correctly? I'm not familiar enough with custom annotations to know, but it seems like the old behavior was working in unit tests.

@michaeljmarshall

Copy link
Copy Markdown
Member

@shoothzj - The test fails because a configuration name including Password is printed in a key. The sensitive field's value is ****, so there is no leak. When I remove Assert.assertFalse(s.contains("Password"));, the test passes.

@hezhangjian

Copy link
Copy Markdown
Member Author

@michaeljmarshall I retest the code. You are right, shall we revert this PR? I can work on it.

@michaeljmarshall

Copy link
Copy Markdown
Member

@shoothzj - thanks for testing it out and confirming! I think it'd be good to revert it. I'm happy to help review and cherry-pick it, if that helps.

hezhangjian pushed a commit to hezhangjian/pulsar that referenced this pull request May 27, 2022
hezhangjian pushed a commit to hezhangjian/pulsar that referenced this pull request Jun 2, 2022
hezhangjian pushed a commit to hezhangjian/pulsar that referenced this pull request Jun 3, 2022
hezhangjian pushed 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants