8000 UCX-3589 SSL certificate setup for UcxUcc by josephkabraham · Pull Request #24 · infinityoneframework/infinity_one · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

UCX-3589 SSL certificate setup for UcxUcc #24

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

Open
wants to merge 13 commits into
base: emetrotel
Choose a base branch
from

Conversation

josephkabraham
Copy link
Collaborator

Implemented as per WebRTC client and tested with the updated self signed certificate.

@josephkabraham josephkabraham self-assigned this Jan 26, 2018
Copy link
Collaborator
@smpallen99 smpallen99 left a comment

Choose a reason for hiding this comment

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

Some changes to address.

@@ -6,6 +6,8 @@ defmodule UcxUcc.Application do
def start(type, args) do
import Supervisor.Spec

UcxUcc.CertManager.set_endpoint_certs! :ucx_ucc, UcxUccWeb.Endpoint
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to move this stuff to the UcxAdapter plugin. You will need to use the plugin's Application callbacks to run this, similar to how you did the Licensing since the modules makes a lot of assumptions that are very specific to our production product.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved this startup function call to ucx_adapter/application.ex

@@ -0,0 +1,96 @@
# Copyright (C) E-MetroTel, 2018 - All Rights Reserved
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move this to plugins/ucx_adapter/lib/ucx_adapter/cert_manager.ex with the UcxAdapter.CertManager module name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved the source to ucx_adapter plugin.


get_cert_file # uses the default #{@ssl_conf_file} file

get_cert_file(:mscs) # gets the config filename from the config
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be :ucx_ucc, not :mscs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated comment.

File.stream!(ssl_conf_file, [], :line)
|> Enum.reduce([], fn(line, acc) ->
line = Regex.replace ~r/#.*/, line, ""
cond do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Few things in this block.

  1. find_key already trims the string. So no need to do it a second time.
  2. Code guidelines
cond do
  value = find_key(@ssl_cert, line) ->
    acc
    |> Keyword.put(:certfile, value)
    |> Keyword.put_new(:cacertfile, value)
  value = find_key(@ssl_cert_key, line) ->
    Keyword.put(acc, :keyfile, value)
  value = find_key(@ssl_ca_cert, line) ->
    Keyword.put(acc, :cacertfile, value))
  true -> acc
end

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated code.

endpoint = Application.get_env(app, endpoint_mod)
https = get_cert_info(app)
|> Enum.reduce(endpoint[:https] || [], fn({k,v}, https) ->
if(Keyword.get(https, k) != nil) do
Copy link
Collaborator

Choose a reason for hiding this comment

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

should never compare against nil. Use is_nil/1. Also no parens for an if statement.

  if is_nil Keyword.get(https, k), do: Keyword.put(https, k, v), else: https

Actually, there is already an API for this. Keyword.put_new/3. This could be as simple as

|> Enum.reduce(endpoint[:https] || [], fn {k, v}, https -> Keyword.put_new(https, k, v) end)

also note that the args of a fn should not be in parens.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated code.

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.

2 participants
0