Skip to content

Use SecurityUtility class - #1213

Merged
maskit merged 3 commits into
apache:masterfrom
maskit:use_security_utility
Feb 11, 2018
Merged

Use SecurityUtility class#1213
maskit merged 3 commits into
apache:masterfrom
maskit:use_security_utility

Conversation

@maskit

@maskit maskit commented Feb 10, 2018

Copy link
Copy Markdown
Member

Motivation

  • Duplicate code makes maintenance hard

Modifications

  • Rename createNettySslContext to createNettySslContextForClient
  • Add createNettySslContextForServer
  • Use createNettySslContextForServer from PulsarChannelInitializer and ServiceChannelInitializer

Result

  • Less duplicate code

@maskit maskit added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Feb 10, 2018
@maskit maskit added this to the 1.22.0-incubating milestone Feb 10, 2018
@maskit maskit self-assigned this Feb 10, 2018
maskit
maskit commented Feb 10, 2018
View reviewed changes
Comment thread pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ServiceChannelInitializer.java Outdated
// allows insecure connection
builder.trustManager(InsecureTrustManagerFactory.INSTANCE);
SslContext sslCtx = builder.clientAuth(ClientAuth.OPTIONAL).build();
SslContext sslCtx = SecurityUtility.createNettySslContextForClient(true,

@maskit maskit Feb 10, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Note that the first argument forallowInsecureConnection is true because the original code always allows insecure connection regardless of configuration. If it wasn't intentional, it need to be fixed.

@rdhabalia rdhabalia Feb 10, 2018

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.

can we also add comment SecurityUtility.createNettySslContextForClient(true\* to allow InsecureConnection*\,..)

@maskit

maskit commented Feb 10, 2018

Copy link
Copy Markdown
Member Author

I know SecurityUtility class itself should be refactored too but I'll do that later to make this PR simple and easy to review.

@maskit maskit mentioned this pull request Feb 10, 2018
Merged
rdhabalia
rdhabalia reviewed Feb 10, 2018
View reviewed changes

@rdhabalia rdhabalia left a comment

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.

LGTM..

Comment thread pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ServiceChannelInitializer.java Outdated
// allows insecure connection
builder.trustManager(InsecureTrustManagerFactory.INSTANCE);
SslContext sslCtx = builder.clientAuth(ClientAuth.OPTIONAL).build();
SslContext sslCtx = SecurityUtility.createNettySslContextForClient(true,

@rdhabalia rdhabalia Feb 10, 2018

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.

can we also add comment SecurityUtility.createNettySslContextForClient(true\* to allow InsecureConnection*\,..)

merlimat
merlimat approved these changes Feb 10, 2018
View reviewed changes

@merlimat merlimat left a comment

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.

👍

@maskit
maskit merged commit 2c0616e into apache:master Feb 11, 2018
@maskit maskit mentioned this pull request Feb 11, 2018
Merged
@maskit
maskit deleted the use_security_utility branch October 23, 2018 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rdhabalia rdhabalia rdhabalia left review comments
@merlimat merlimat merlimat approved these changes
@massakam massakam Awaiting requested review from massakam

Assignees

@maskit maskit

Labels

type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages

Projects

None yet

Milestone

1.22.0-incubating

Development

Successfully merging this pull request may close these issues.

3 participants

@maskit @merlimat @rdhabalia