-
Notifications
You must be signed in to change notification settings - Fork 9
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
base: emetrotel
Are you sure you want to change the base?
UCX-3589 SSL certificate setup for UcxUcc #24
Conversation
There was a problem hiding this 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.
lib/ucx_ucc/application.ex
Outdated
@@ -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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
lib/ucx_ucc/ucx_ucc_cert_manager.ex
Outdated
@@ -0,0 +1,96 @@ | |||
# Copyright (C) E-MetroTel, 2018 - All Rights Reserved |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
lib/ucx_ucc/ucx_ucc_cert_manager.ex
Outdated
|
||
get_cert_file # uses the default #{@ssl_conf_file} file | ||
|
||
get_cert_file(:mscs) # gets the config filename from the config |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated comment.
lib/ucx_ucc/ucx_ucc_cert_manager.ex
Outdated
File.stream!(ssl_conf_file, [], :line) | ||
|> Enum.reduce([], fn(line, acc) -> | ||
line = Regex.replace ~r/#.*/, line, "" | ||
cond do |
There was a problem hiding this comment.
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.
find_key
already trims the string. So no need to do it a second time.- 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated code.
lib/ucx_ucc/ucx_ucc_cert_manager.ex
Outdated
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated code.
Implemented as per WebRTC client and tested with the updated self signed certificate.