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 optional TLS SNI initiator support #277

Merged
merged 26 commits into from
Apr 3, 2020
Merged

Add optional TLS SNI initiator support #277

merged 26 commits into from
Apr 3, 2020

Conversation

youurayy
Copy link
Contributor

@youurayy youurayy commented Mar 27, 2020

Fixes #276.

@the-thing
Copy link
Contributor

Is there a chance to wring unit test for it? I guess doing end to end unit test is not possible without setting up virtual hosts, isn't it?

@chrjohn
Copy link
Member

chrjohn commented Mar 28, 2020

I am not familiar with this, but adding a flag to disable SNI is not necessary because it will be ignored by counterparties not supporting it?? Really really sure? ;)

@youurayy
Copy link
Contributor Author

youurayy commented Mar 29, 2020

@chrjohn I am not familiar with this, but adding a flag to disable SNI is not necessary because it will be ignored by counterparties not supporting it?? Really really sure? ;)

I'm pretty sure, but yeah, still. Will make it opt-in then?

@youurayy youurayy changed the title Activate TLS SNI support by default when using TLS Add optional TLS SNI initiator support when using TLS Mar 30, 2020
@youurayy
Copy link
Contributor Author

Not sure what to do with the failing LGTM for JavaScript, the log says:

image

I assume this is because configuration.html was edited, and LGTM cannot extract any JavaScript from it to perform checks on.

One way to solve this would be to add a ./lgtm.yaml and exclude the doc dir, since no files there use any JavaScript:

extraction:
  javascript:
    index:
      exclude:
        - quickfixj-core/scr/main/doc
      filters:
        - exclude: "**/*.html"

as per https://lgtm.com/help/lgtm/javascript-extraction.

Not sure if that should be part of this PR, let me know pls.

@chrjohn
Copy link
Member

chrjohn commented Mar 31, 2020

Please add the LGTM config. IMHO you could also disable JavaScript checking altogether, but don't know if that is possible.

Edit: you got a typo in the exclude section. Should be src directory.

@chrjohn chrjohn added this to the QFJ 2.2.0 milestone Mar 31, 2020
@chrjohn
Copy link
Member

chrjohn commented Mar 31, 2020

JavaScript analysis failed again. But for me this should not block the PR from merging. If you don't have any idea how to fix it let me know.

@youurayy
Copy link
Contributor Author

@chrjohn I'm in the process of trying to figure it out -- don't want to be guessing so I'm reading their docs now. There's lot of .js files under /dist/tools/.. (they seem correctly ignored), so I think simply excluding all *.html won't do it. I'll try to commit a fix soon.

@youurayy
Copy link
Contributor Author

It seems that the LGTM JavaScript extractor will always complain if it fails to extract at least one file: https://github.com/Semmle/ql/blob/master/javascript/extractor/src/com/semmle/js/extractor/AutoBuild.java#L1088

@chrjohn
Copy link
Member

chrjohn commented Mar 31, 2020

We don't have any lgtm.yml in place yet so it seems that by default JavaScript is tested. Could we only specify Java to skip JS extraction? Cf here: https://github.com/elastic/apm-agent-java/blob/master/.lgtm.yml

@chrjohn
Copy link
Member

chrjohn commented Mar 31, 2020

Dang..

@youurayy
Copy link
Contributor Author

Okay, giving up.

Somebody should check if it's possible to turn off the LGTM JavaScript integration in the repo settings.

Alternatively, the lgtm.yml could perhaps be tweaked using the path_classifiers: section, but this change will not be applied from a PR, must be applied on the repo directly:
image

The last lgtm.yml I tried was:

extraction:
  javascript:
    before_index:
      - export LGTM_TRAP_CACHE=
    index:
      exclude:
        - quickfixj-core
        - quickfixj-codegenerator
      filters:
        - exclude: "*.html"
        - exclude: "*.js"
        - exclude: "*.yml"
        - exclude: "**/*.html"
        - exclude: "**/*.js"
        - exclude: "**/*.yml"
      typescript: none

(the LGTM_TRAP_CACHE var reset was an attempt to disable the externs extraction)

What was weird, the extraction was failing even when a phony <script>..</script> block was inserted into one of the files which were 100% processed by the extractor.

@chrjohn chrjohn changed the title Add optional TLS SNI initiator support when using TLS Add optional TLS SNI initiator support Apr 1, 2020
@chrjohn
Copy link
Member

chrjohn commented Apr 1, 2020

I'll check with LGTM to disable the JavaScript support since I cannot disable it in the project settings. Seems to be a known problem or missing feature: https://discuss.lgtm.com/t/how-to-turn-off-language-python-support-for-the-c-project/1388 and https://discuss.lgtm.com/t/disable-javascript-analysis/2150/15

Apart from that I think we are ready to merge this. Thanks for the contribution @youurayy 👍
Thank you @the-thing for your review and comments here and on #276 👍

@chrjohn chrjohn merged commit e53d41b into quickfix-j:master Apr 3, 2020
jli8000 pushed a commit to jli8000/quickfixj that referenced this pull request Sep 22, 2020
* add address to SNI init log output
* add UseSNI opt-in parameter
* add doc for the UseSNI config parameter
* change to parametrized logging
* minor changes to comments and logging
* corrected SNI (indication) in doc

Co-authored-by: Christoph John <christoph.john@macd.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add TLS SNI support
3 participants