Skip to content

Conversation

@HashidaTKS
Copy link
Contributor

@HashidaTKS HashidaTKS commented Apr 26, 2023

Which issue(s) this PR fixes:

N/A

What this PR does / why we need it:

複数のクライアント証明書が存在する時に、どの証明書を使うかを選択可能にする。
一度選択した証明書は、同じサイトに対してChronosを再起動するまで同じものが使われる。
別のタブでアクセスしても、同じ証明書が使用される。
(Chronos側で何かをしているわけではなく、CEF側の機能でそうなっている。)

How to verify the fixed issue:

The steps to verify:

  1. https://www.cacert.org/index.php?id=3&lang=ja
    で配布されているCAcertの証明書をルート認証局証明書としてインポートする。
  2. https://x509.w0.dk/
    の「Create a '*.p12' file with OpenSSL」配にある
    「john.doe_example.org.p12」のリンクからファイルをダウンロードする。
  3. ダウンロードした「john.doe_example.org.p12」をクライアント証明書として
    インポートする。
    (パスワード入力を求められるが、何も入力せずそのまま確定する。)
  4. https://sx509.w0.dk/
    を開く。
    • Webサイト訪問時に、サイト証明書が未知の認証局によって発行された物である
      旨のエラーが表示され、Webサイトの内容を閲覧できない場合には、
      警告を無視してWebサイトを表示するよう操作する必要がある。

Expected result:

以下のような、どのクライアント証明書を使用するかの選択ダイアログが表示されること。

image

「Jhon Doe」を選択すると、認証に成功して、認証に使われた証明書の情報が表示されること。

@HashidaTKS
Copy link
Contributor Author

OnSelectClientCertificateで複数の証明書がきていることを確認したところまで。
これをUIで選択させる必要がある。

@ashie ashie added this to the Release-2023Q4 milestone May 10, 2023
@HashidaTKS HashidaTKS force-pushed the support-multi-credentials branch from f0bf684 to 744389f Compare June 19, 2023 05:01
#define IDD_DLG_AUTH_PWC 224
#define IDI_ICON2 225
#define IDI_ICON3 226
#define IDS_STRING_STARTING_BROWSER 227
Copy link
Contributor Author

@HashidaTKS HashidaTKS Jun 19, 2023

Choose a reason for hiding this comment

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

自動生成されるファイル。今まで自動整形していたが、自動整形しても大量の差分が出るし、このファイルを直接扱うことはないはずなので、今後は自動整形の対象から外したい。

@HashidaTKS HashidaTKS force-pushed the support-multi-credentials branch from 26adb6c to 1192ae8 Compare June 19, 2023 05:48
resource.h Outdated
#define ID_ERROR_MSG_CERT_CONTAINS_ERRORS 345
#define ID_ERROR_MSG_CERT_NO_REVOCATION_MECHANISM 346
#define IDS_STRING_STARTING_BROWSER 227
#define IDD_DLG_CERTIFICATION 227
Copy link
Contributor Author

Choose a reason for hiding this comment

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

227番が二つある。大丈夫なのだろうか。。。

@HashidaTKS HashidaTKS force-pushed the support-multi-credentials branch from 63462e8 to 43ab998 Compare June 20, 2023 08:25
@HashidaTKS
Copy link
Contributor Author

HashidaTKS commented Jun 20, 2023

大枠はできた。

TODO:

  • i18n対応
  • その他ダイアログに表示すべき項目がないか確認
  • ソースファイルがUTF8になっているものを直す

#undef APSTUDIO_READONLY_SYMBOLS

/////////////////////////////////////////////////////////////////////////////
// ���{�� (�j���[�g����) resources
Copy link
Contributor Author

Choose a reason for hiding this comment

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

自動生成により生じた差分。このままとしたい。

return false;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

自動整形による差分。

@HashidaTKS
Copy link
Contributor Author

レビュー可能な状態となったので、お手隙の際にレビューをお願いします。

@HashidaTKS HashidaTKS marked this pull request as ready for review June 21, 2023 04:46
private:
CString GetSerialNumberAsHexString(const CefRefPtr<CefX509Certificate> certificate);
CString GetTimeString(const CefTime& value);
CString GetPrincipalString(const CefRefPtr<CefX509CertPrincipal> principal);
Copy link
Contributor Author

@HashidaTKS HashidaTKS Jun 21, 2023

Choose a reason for hiding this comment

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

このあたり、CefRefPrtなどのCefの定義を使わないような作りのほうが良さそうにも思うが、結局CefX509CertPrincipalなどのCefの定義を渡さなければならないので、とりあえずCefRefPtrを渡すようなつくりになっている。

@HashidaTKS HashidaTKS changed the title Add implementation of OnSelectClientCertificate Add support for mutilple certificate Jun 21, 2023
@ashie ashie self-requested a review June 22, 2023 01:59
virtual ~CDlgCertification();

CComboBox certificationComboBox;
int* m_selectedIndex;
Copy link
Member

Choose a reason for hiding this comment

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

これがポインターである理由って何かありますか?
ポインターでない方がコードが分かりやすくなるような気がします。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

このあたりの事情について、以下の箇所にコメントしているのですが、Web上だと文字化けしていてわからないですね...。

https://github.com/ThinBridge/Chronos/pull/59/files#diff-81f6b939c1ea55e2db32c26a260a87e76b733cfab03492ba852f722c30c2e91eR169

	// DoModalなどで結果を返し終えた後、certificationComboBoxなどが解放されてしまって参照できない。
	// なので、このタイミングでクラス変数に結果を代入しておく。

また、最初はこのm_selectedIndexOnCbnSelchangeCertificationComboで更新するようにしていたのですが、DoModal後のダイアログ解放のタイミングでもOnCbnSelchangeCertificationComboが呼ばれてしまい、その時certificationComboBox.GetCurSel()が0になり、最終的にDoModal実行後には常にm_selectedIndexが0になってしまっていたので、上手く行きませんでした。

現状がイマイチという点につきましては賛同ですので、もう少し良い方法がないか探ってみます。

Copy link
Member

Choose a reason for hiding this comment

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

なるほど。

だとしてもポインターである必要はない(DoModal後もdlgそのものが解放されるわけではないと思うのので、m_selectedIndexは参照できるはずであり、直接参照すれば良く、ローカル変数を介する必要がない)ような気がしたのですが、その点ではどうでしょう?
(確かめないで言っているので、解放されてしまうようでしたらごめんなさい)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

m_selectedIndexも解放されてしまうと思い込んでいたのですが、試したところ解放されなかったので、直接参照するような作りに変更します!ありがとうございます!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

m_selectedIndexをprivateなメンバーにしました。

また、結局m_hostm_certificatesも外部から参照されていないので、こちらもprivateなメンバーにしてコンストラクタで渡すように変更しました。

@ashie ashie merged commit 7f78081 into ThinBridge:master Jun 22, 2023
This was referenced Jun 23, 2023
@HashidaTKS HashidaTKS deleted the support-multi-credentials branch October 12, 2023 04:50
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