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

Add "mechanism" in output.kafka to support SCRAM-SHA-512 and SCRAM-SHA-256 #12867

Merged
merged 8 commits into from
Mar 2, 2020

Conversation

zvictorino
Copy link
Contributor

@zvictorino zvictorino commented Jul 11, 2019

New config key mechanism was introduced.
How to use it:

output.kafka:
  codec.format:
    string: '%{[@timestamp]} %{[message]}'

  hosts: ["localhost:9092"]
  topic: 'mytopic'
  partition.round_robin:
    reachable_only: false
  required_acks: 1
  compression: gzip
  max_message_bytes: 1000000
  username: user
  password: pass
  sasl.mechanism: SCRAM-SHA-512

… and SCRAM-SHA-256 mechanism.

How to use it:
```
output.kafka:
  codec.format:
    string: '%{[@timestamp]} %{[message]}'

  hosts: ["localhost:9092"]
  topic: 'mytopic'
  partition.round_robin:
    reachable_only: false
  required_acks: 1
  compression: gzip
  max_message_bytes: 1000000
  username: user
  password: pass
  mechanism: SCRAM-SHA-512
```
@zvictorino zvictorino requested a review from a team as a code owner July 11, 2019 13:16
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

return
}

func (x *XDGSCRAMClient) Done() bool {

Choose a reason for hiding this comment

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

exported method XDGSCRAMClient.Done should have comment or be unexported

return nil
}

func (x *XDGSCRAMClient) Step(challenge string) (response string, err error) {

Choose a reason for hiding this comment

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

exported method XDGSCRAMClient.Step should have comment or be unexported

scram.HashGeneratorFcn
}

func (x *XDGSCRAMClient) Begin(userName, password, authzID string) (err error) {

Choose a reason for hiding this comment

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

exported method XDGSCRAMClient.Begin should have comment or be unexported

var SHA256 scram.HashGeneratorFcn = func() hash.Hash { return sha256.New() }
var SHA512 scram.HashGeneratorFcn = func() hash.Hash { return sha512.New() }

type XDGSCRAMClient struct {

Choose a reason for hiding this comment

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

exported type XDGSCRAMClient should have comment or be unexported

)

var SHA256 scram.HashGeneratorFcn = func() hash.Hash { return sha256.New() }
var SHA512 scram.HashGeneratorFcn = func() hash.Hash { return sha512.New() }

Choose a reason for hiding this comment

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

exported var SHA512 should have comment or be unexported

)

var SHA256 scram.HashGeneratorFcn = func() hash.Hash { return sha256.New() }

Choose a reason for hiding this comment

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

exported var SHA256 should have comment or be unexported

@chlunde
Copy link

chlunde commented Aug 14, 2019

@jsoriano this would fix #8387

@jsoriano
Copy link
Member

@zvictorino thanks for opening this PR! Tests are failing because there is a missing package, you will need to add it with govendor fetch.
It'd be also nice to add some docs for the new option, but we can add it in a follow up.

@urso
Copy link

urso commented Aug 14, 2019

let's name the setting sasl_mechanism or create an sasl namespace so the setting becomes sasl.mechanism (assuming we might add more settings in the future).

The setting will also need some docs.

Almost all symbols are private to the package, don't export them.

@@ -58,6 +89,7 @@ type kafkaConfig struct {
Username string `config:"username"`
Password string `config:"password"`
Codec codec.Config `config:"codec"`
Mechanism string `config:"mechanism"`
Copy link

Choose a reason for hiding this comment

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

this calls for an enum like type (over string):

type saslMechanism string

const (
  saslTypePlaintext = sarama.SASLTypePlaintext
  ...
)

func (m *saslMechanism) Unpack(in string) error {
  in = strings.ToUpper(in)  // try not to force users to use all upper case
  switch in {
  case saslTypePlaintext, ...:
    *m = saslMechanism(in)
  default:
    return fmt.Errorf("not valid mechanism '%v', only supported with PLAIN|SCRAM-SHA-512|SCRAM-SHA-256", in)
  }
  return nil   
}

The unpack method will be called by (*Config).Unpack. If it fails the full config name and the config file the setting comes from will be reported.

There is another 'string' comparison in order to fill in the sarama SASL settings. Having magic strings in different places is a good way to introduce Bugs (future developer might have typo when adding another mechanism). Better use an enum and use contanstants (compiler will complain if there is a typo). By having our own type we can also do this (optional):

func (m saslMechanism) configureSarama(... <other input>?, config *sarama.Config) error {
  switch m {
  case saslScramSHA256:
    ...

  case sasl<type>:
    ...

  default:
    panic(<we shouldn't get here, assuming no developer messed up>)
  }
}

@elasticcla
Copy link

Hi @zvictorino, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in your Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

Move some codes to separate file.
libbeat/outputs/kafka/scram.go Show resolved Hide resolved
return nil
}

func (x *XDGSCRAMClient) Step(challenge string) (response string, err error) {

Choose a reason for hiding this comment

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

exported method XDGSCRAMClient.Step should have comment or be unexported

scram.HashGeneratorFcn
}

func (x *XDGSCRAMClient) Begin(userName, password, authzID string) (err error) {

Choose a reason for hiding this comment

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

exported method XDGSCRAMClient.Begin should have comment or be unexported

var SHA256 scram.HashGeneratorFcn = func() hash.Hash { return sha256.New() }
var SHA512 scram.HashGeneratorFcn = func() hash.Hash { return sha512.New() }

type XDGSCRAMClient struct {

Choose a reason for hiding this comment

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

exported type XDGSCRAMClient should have comment or be unexported

)

var SHA256 scram.HashGeneratorFcn = func() hash.Hash { return sha256.New() }
var SHA512 scram.HashGeneratorFcn = func() hash.Hash { return sha512.New() }

Choose a reason for hiding this comment

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

exported var SHA512 should have comment or be unexported

"github.com/xdg/scram"
)

var SHA256 scram.HashGeneratorFcn = func() hash.Hash { return sha256.New() }

Choose a reason for hiding this comment

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

exported var SHA256 should have comment or be unexported

@@ -0,0 +1,37 @@
// https://github.com/Shopify/sarama/blob/master/examples/sasl_scram_client/scram_client.go

Choose a reason for hiding this comment

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

package comment should be of the form "Package kafka ..."

@ph
Copy link
Contributor

ph commented Oct 18, 2019

Hello @zvictorino this is a really nice addition, its there anything we could help to move it forward with you?

@jsoriano
Copy link
Member

jenkins, test this

@armysheng
Copy link

can anyone merge this pull request plz ?

@jsoriano
Copy link
Member

@zvictorino there is a merge conflict, could you please update the branch with master? Thanks!

@jsoriano jsoriano self-assigned this Jan 20, 2020
@jsoriano jsoriano added the Team:Services (Deprecated) Label for the former Integrations-Services team label Feb 6, 2020
@cla-checker-service
Copy link

cla-checker-service bot commented Feb 26, 2020

💚 CLA has been signed

@jsoriano
Copy link
Member

ok to test

@jsoriano
Copy link
Member

jenkins, test this again please

@jsoriano
Copy link
Member

Code LGTM, but there would be some pending things. We would need a changelog entry, docs for the new options, and some tests.

@zvictorino let me know if you can continue with this, if not I am ok with merging this as is and create an issue with some follow up tasks.

Thanks!

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Merging this as is by now, but not backporting to 7.x, I will create an issue for follow ups. Thanks @zvictorino!

@jsoriano jsoriano merged commit e935b26 into elastic:master Mar 2, 2020
@jsoriano jsoriano added the v8.0.0 label Mar 2, 2020
@jsoriano
Copy link
Member

jsoriano commented Mar 2, 2020

Follow up issue for this: #16723

jsoriano added a commit that referenced this pull request Dec 14, 2020
jsoriano pushed a commit to jsoriano/beats that referenced this pull request Dec 14, 2020
jsoriano added a commit to jsoriano/beats that referenced this pull request Dec 14, 2020
@jsoriano jsoriano added v7.11.0 test-plan Add this PR to be manual test plan and removed test-plan Add this PR to be manual test plan labels Dec 14, 2020
jsoriano added a commit that referenced this pull request Dec 14, 2020
… SCRAM-SHA-512 and SCRAM-SHA-256 (#23110)

Add "mechanism" in output.kafka to support  SCRAM-SHA-512 and  SCRAM-SHA-256.

(cherry picked from commit e935b26)
(cherry picked from commit 87ff5c0)

Co-authored-by: zvictorino <zvictorino@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libbeat :Outputs review Team:Services (Deprecated) Label for the former Integrations-Services team v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants