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

TlsContextConfiguration does not respect default values when deserialized from YAML with Jackson #1209

Closed
sleberknight opened this issue Oct 20, 2024 · 0 comments · Fixed by #1210
Assignees
Labels
bug Something isn't working
Milestone

Comments

@sleberknight
Copy link
Member

sleberknight commented Oct 20, 2024

When deserializing TlsContextConfiguration from YAML using Jackson, field default values are not respected and the deserialized object contains the default value for that type of field (null) for reference types, and the default value for primitive types (e.g., false for boolean).

The tests have always used SnakeYAML to deserialize, and they've always passed. And in all of our Dropwizard services, we use dropwizard-config-providers to provide default values for key/trust stores and thus we don't have explicit configuration for TlsContextConfiguration in our YAML.

But recently for some new development work, I needed to have an explicit configuration for TlsContextConfiguration in a YAML configuration for a Dropwizard application, and found that it was not respecting the default values for keyStoreType and trustStoreType (JKS), for protocol (TLSv1.2), and for verifyHostname (true). Instead, keyStoreType, trustStoreType, and protocol were all null and verifyHostname was false. And yet all the tests pass in TlsContextConfigurationTest! (I guess tests are worthless after all 🤣 )

Long story short, it was because of the Lombok annotations used in TlsContextConfiguration.

TlsContextConfiguration has the following Lombok annotations at the class-level:

@Getter
@Setter
@Builder
@NoArgsConstructor
@AllArgsConstructor(access = AccessLevel.PRIVATE)  // for Builder (b/c also need no-args constructor)

It also uses @Builder.Default on protocol, keyStoreType, trustStoreType, and verifyHostname. Over time we've found that this combination of Lombok annotations does not work as you would expect in some situations, depending on whether you use the builder, the no-args constructor, or the all-args constructor. The builder and no-args constructor work as expected, while the all-args constructor does not.

But why did the tests pass using SnakeYAML, and why did the deserialization not work as expected in the Dropwizard application? When I de-lomboked TlsContextConfiguration, the fields each still have their default value, so when the no-args constructor is used along with setter methods, the default values are respected unless overwritten. But the all-args constructor does not respect the default values. Here is the de-lomboked all-args constructor code:

    @java.beans.ConstructorProperties({ "protocol", "provider", "keyStorePath", "keyStorePassword", "keyStoreType", "keyStoreProvider", "trustStorePath", "trustStorePassword", "trustStoreType", "trustStoreProvider", "trustSelfSignedCertificates", "verifyHostname", "disableSniHostCheck", "supportedProtocols", "supportedCiphers", "certAlias" })
    private TlsContextConfiguration(@NotBlank String protocol, String provider, String keyStorePath, String keyStorePassword, @NotBlank String keyStoreType, String keyStoreProvider, @NotBlank String trustStorePath, @NotNull String trustStorePassword, @NotBlank String trustStoreType, String trustStoreProvider, boolean trustSelfSignedCertificates, boolean verifyHostname, boolean disableSniHostCheck, List<String> supportedProtocols, List<String> supportedCiphers, String certAlias) {
        this.protocol = protocol;
        this.provider = provider;
        this.keyStorePath = keyStorePath;
        this.keyStorePassword = keyStorePassword;
        this.keyStoreType = keyStoreType;
        this.keyStoreProvider = keyStoreProvider;
        this.trustStorePath = trustStorePath;
        this.trustStorePassword = trustStorePassword;
        this.trustStoreType = trustStoreType;
        this.trustStoreProvider = trustStoreProvider;
        this.trustSelfSignedCertificates = trustSelfSignedCertificates;
        this.verifyHostname = verifyHostname;
        this.disableSniHostCheck = disableSniHostCheck;
        this.supportedProtocols = supportedProtocols;
        this.supportedCiphers = supportedCiphers;
        this.certAlias = certAlias;
    }

When I ran the tests through the debugger, I found that SnakeYAML uses the no-args constructor and then calls setter methods, while Jackson (which is what Dropwizard uses under the hood), uses the all-args constructor. When Jackson parses YAML that does not contain values for all properties, it passes default values to the all-args constructor, e.g., null for reference types and false for primitive boolean. This is why the fields with default values are incorrect in the deserialized object when using Jackson. I'm sure there are some ways to configure Jackson to force it to use the no-args constructor but the easier solution is to simply remove the top-level Lombok constructor annotations and the @Builder.Default ones, and write the constructors manually. This way the all-args constructor can respect the field default values.

@sleberknight sleberknight added the bug Something isn't working label Oct 20, 2024
@sleberknight sleberknight added this to the 4.5.1 milestone Oct 20, 2024
@sleberknight sleberknight self-assigned this Oct 20, 2024
sleberknight added a commit that referenced this issue Oct 20, 2024
* Replace Lombok constructor annotations with actual constructors
  such that the all-args constructor respects the default values.
* Remove the Builder.Default methods since the default values
  are now handled in the all-args constructor.
* The no-args constructor calls the all-args constructor with
  all null values so that defaults are in one place (instead of
  on field declarations and in the all-args constructor).
* The all-args constructor sets the appropriate default values
  when it does not receive a value for a given field. It also
  has the ConstructorProperties annotation to help deserializers
  know how the arguments map to fields.
* Update tests for TlsContextConfiguration and
  SecureEndpointsConfiguration so that deserialization from YAML is
  tested for both TlsContextConfiguration and SSLContextConfiguration
  (SecureEndpointsConfiguration extends it) using SnakeYAML, Jackson,
  and Dropwizard. Though Dropwizard uses Jackson and should be the
  same as using "plain Jackson," we want to make 100% sure Dropwizard
  isn't doing any kind of customization in Jackson that might change
  how it deserializes the YAML.
* Add new helper methods in YamlTestHelper and rename the existing
  method so that the method names clearly express how they perform
  the deserialization, i.e., using SnakeYAML or Jackson.

Closes #1209
sleberknight added a commit that referenced this issue Oct 20, 2024
* Replace Lombok constructor annotations with actual constructors such
that the all-args constructor respects the default values.
* Remove the Builder.Default methods since the default values are now
handled in the all-args constructor.
* The no-args constructor calls the all-args constructor with all null
values so that defaults are in one place (instead of on field
declarations and in the all-args constructor).
* The all-args constructor sets the appropriate default values when it
does not receive a value for a given field. It also has the
ConstructorProperties annotation to help deserializers know how the
arguments map to fields.
* Update tests for TlsContextConfiguration and
SecureEndpointsConfiguration so that deserialization from YAML is tested
for both TlsContextConfiguration and SSLContextConfiguration
(SecureEndpointsConfiguration extends it) using SnakeYAML, Jackson, and
Dropwizard. Though Dropwizard uses Jackson and should be the same as
using "plain Jackson," we want to make 100% sure Dropwizard isn't doing
any kind of customization in Jackson that might change how it
deserializes the YAML.
* Add new helper methods in YamlTestHelper and rename the existing
method so that the method names clearly express how they perform the
deserialization, i.e., using SnakeYAML or Jackson.

Closes #1209
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant