SASL EXTERNAL implementation #1735

DenysGonchar · 2018-02-19T14:25:40Z

Initial implementation of SASL EXTERNAL

arcusfelis · 2018-02-19T14:37:37Z

include/ejabberd_c2s.hrl

@@ -27,12 +27,13 @@
                tls_required = false  :: boolean(),
                tls_enabled = false   :: boolean(),
                tls_options = [],
+                verify                :: verify_none | verify_peer,


sasl_verify or something more self-describing good to have.
Should we replace this record with a map eventually?

At least with my client we have everything else field with a map for any extra stuff.

verify_none & verify_peer are the standard atoms in erlang's ssl and fast_tls. so let it stay standard,
if we want to change record to map - let's do it in another CR. this one is already too big

I was talking about #state.verify field name.
Values are fine. But field name is very confusing.

Can be

sasl_verify :: verify_none | verify_peer

or

sasl_ssl_verify :: verify_none | verify_peer

or

sasl_tls_verify :: verify_none | verify_peer

Also, there should be info, that this field is initialized based on listener settings.

arcusfelis · 2018-02-19T14:38:47Z

src/cert_utils.erl

@@ -0,0 +1,86 @@
+-module(cert_utils).


Module description comment

E.g. what certificates, what is the primary user of this module...

arcusfelis · 2018-02-19T14:39:31Z

src/cert_utils.erl

+      #'AttributeTypeAndValue'{type = ?'id-at-commonName', value = V} <- AtributesList],
+    CN
+  catch
+    _:_ -> error


report error here, we are loosing stacktrace info.

i'm not gonna log here anything, it's normal when certificate is missing some of the fields, generally it's not an error. if you want to log something - do it in the proper place, it's a duty of the caller

It's not about logging but returning a meaningful error, so the caller will actually know what went wrong.

arcusfelis · 2018-02-19T14:39:54Z

src/cert_utils.erl

+       XmppAddr
+     end || {otherName, #'AnotherName'{'type-id' = ?'id-on-xmppAddr', value = V}} <- SANs]
+  catch
+    _:_ -> []


report an error here

i'm not gonna log here anything, it's normal when certificate is missing some of the fields, generally it's not an error. if you want to log something - do it in the proper place, it's a duty of the caller

Than this code can be rewritten without try ... catch.
Just by expecting missing fields.
There also should be at least two tests: one with missing field, one with not.

.... ideally.

It deserves a separate error return, when a field expected to have a valid XMPP address, does not actually have one. If fields are missing at all, then I think it's OK to return [].

arcusfelis · 2018-02-21T20:05:25Z

src/cert_utils.erl

+    {ok, SANs} = 'OTP-PUB-KEY':decode('SubjectAltName', BinVal),
+    [DNS || {dNSName, DNS} <- SANs]
+  catch
+    _:_ -> []


ok, catching everything is usually bad practice.
I should be used with care and should report errors and stacktrace.
Otherwise we would ignore some invisible errors.

i'm not gonna log here anything, it's normal when certificate is missing some of the fields, generally it's not an error. if you want to log something - do it in the proper place, it's a duty of the caller

try ... catch _:_ construction should not used in code, because it hides all the errors.
Both expected and not.

If there was a typo in SubjectAltName, for example SubjectAltnaen we would never find this out.

I am fine with

case decode(....) of {ok, SANs} -> .... {error, _} -> [] end

On microbenchmarking side: going inside catch construction is slower than just case.

Michael, this module will be covered with the tests. that will be done in the next PR because we need to change our SSL certs generation suit, and that is big chunk of work. generally i did manual tests for this code so it's ok to archive it. this code provides only ASN1 parsing, it is ok to to use try/catch here. i'm not gonna change it. period. this comment is related to all the parsing functions.

It's not about doing a typo in a code or tests but lack of any meaningful error return here will make it more difficult to debug problems with real-world certificates. I'd even prefer a lack of try..catch rather than hiding errors.

arcusfelis · 2018-02-21T20:05:50Z

src/cert_utils.erl

+  end.
+
+
+-spec get_cert_domains(certificate()) -> [any()].


more specific type than any().

arcusfelis · 2018-02-21T20:06:11Z

src/cert_utils.erl

+  end.
+
+
+-spec get_xmpp_addresses(certificate()) -> [any()].


more specific type.

arcusfelis · 2018-02-21T20:08:02Z

src/cert_utils.erl

+  Addresses = get_xmpp_addresses(Cert),
+  Domains = get_dns_addresses(Cert),
+  lists:flatten(get_lserver_from_addr(CN,false) ++
+    [get_lserver_from_addr(Addr,true) || Addr <- Addresses, is_binary(Addr)] ++


whitespaces , .

There is nothing in dev-handbook about whitespaces, but generally we use them.

arcusfelis · 2018-02-21T20:16:01Z

src/cert_utils.erl

+  Val.
+
+
+get_lserver_from_addr(V, UTF8) when is_binary(V); is_list(V) ->


it can be two functions, UTF8 argument is not needed.
Also, it returns list if failed to parse - should be in a comment.

returns a list of binaries. The length of the list is one or zero.

I'm not sure if it really requires a comment. Spec would be useful on the other hand.

arcusfelis · 2018-02-21T20:16:48Z

src/cert_utils.erl

+  CN = get_common_name(Cert),
+  Addresses = get_xmpp_addresses(Cert),
+  Domains = get_dns_addresses(Cert),
+  lists:flatten(get_lserver_from_addr(CN,false) ++


while lists:flatten works here, lists:append is more fitting, because we don't have nested lists.

actually it is [bitstring] ++ [[bitstring],...] ++ [[bitstring],...]

arcusfelis · 2018-02-21T20:23:34Z

src/cyrsasl.erl

@@ -122,9 +122,9 @@ listmech(Host) ->
                             {'EXIT', {undef, [{Module, store_type, []} | _]}} ->
                                 ?WARNING_MSG("~p doesn't implement the function store_type/0",
                                              [Module]),
-                                 [];
+                                 [{'/=','$2', cert}];


I would love to see a comment like: "we don't need any type of password for cert mechanism".

Wat? It's more like "we don't want to offer cert mechanism if store type is unknown... but why exactly?

correct, we want to offer external mechanism only if store type is scram. it's done with thought regarding future extension of DB based auth backends. we can extend them with certificates support only if password is scrammed, other-way we can't distinguish values between passwords and certs

arcusfelis · 2018-02-21T20:24:46Z

src/cyrsasl_external.erl

@@ -0,0 +1,61 @@
+-module(cyrsasl_external).


Header, what is while for, how works.
What's the difference between this and cyrsasl_plain.

arcusfelis · 2018-02-21T20:28:55Z

src/cyrsasl_external.erl

+
+-behaviour(cyrsasl).
+
+-record(state, {creds}).


saslext_state would be a better name.
Because sometimes we do load records definitions into shell, and it works better, if each record have a unique name.

But maybe consider to use maps.
creds should be typed and commented.

We use state for record name in so many places, that I think it should be a coordinated refactoring effort (if we would like to do it at all).

But 👍 for typed creds.

arcusfelis · 2018-02-21T20:29:39Z

src/cyrsasl_external.erl

+               Creds :: mongoose_credentials:t()) -> {ok, tuple()}.
+mech_new(_Host, Creds) ->
+  case mongoose_credentials:get(Creds,client_cert,undefined) of
+    undefined -> {ok, #state{creds = invalid_cert}};


arcusfelis · 2018-02-21T20:31:22Z

src/cyrsasl_external.erl

+      DerCert = public_key:pkix_encode('Certificate', Cert, plain),
+      PemCert = public_key:pem_encode([{'Certificate', DerCert, not_encrypted}]),
+      ExtraFields = get_common_name(Cert) ++ get_xmpp_addresses(Cert),
+      {ok, #state{creds = mongoose_credentials:extend(Creds, [ {pem_cert, PemCert},


[ {pem_cert, ..... can go into a separate variable. ExtraFields2 or something.

there is dedicated variable ExtraFields, because it can be [ ], or list of definitions. and that is not the case for DerCert and PemCert.

OK

ExtraFields = get_common_name(Cert) ++ get_xmpp_addresses(Cert), AllFields = [{pem_cert, PemCert}, {der_cert, DerCert} | ExtraFields], mongoose_credentials:extend(Creds, AllFields)

I need to know that the second argument of extend is fields just by looking at the code.

IMO it is fine as it is now and looks clear. :)

arcusfelis · 2018-02-21T20:32:11Z

src/cyrsasl_external.erl

+-spec mech_new(Host :: ejabberd:server(),
+               Creds :: mongoose_credentials:t()) -> {ok, tuple()}.
+mech_new(_Host, Creds) ->
+  case mongoose_credentials:get(Creds,client_cert,undefined) of


whitespaces , .

arcusfelis · 2018-02-21T20:33:14Z

src/cyrsasl_external.erl

+-spec mech_new(Host :: ejabberd:server(),
+               Creds :: mongoose_credentials:t()) -> {ok, tuple()}.
+mech_new(_Host, Creds) ->
+  case mongoose_credentials:get(Creds,client_cert,undefined) of


I would like to know on which step we add client_cert when reading the code.
And if the client_cert is missing, the client is considered unauthorized.

if client cert is missing, client can try another SASL mechanisms, it's not proposed to use SASL EXTERNAL.

Is it possible for a client to reach cyrsasl_external:new without certificate present anyway?

@arcusfelis
It's pretty intuitive that the certificate is provided by c2s. And line 36 is not that far and it explains everything.

no, it's not possible, see ejabberd_c2s, it filters out external mechanism if clients certificate is not provided.

Which part of the XEP covers a case when the EXTERNAL mechanism is offered but client hasn't provided any certificate?

ok, you right, it's not XEP178, it's RFC6120. see 6.3.4. Mechanism Offers

However, the receiving entity MAY offer the SASL EXTERNAL mechanism under other circumstances, as well.

But we do not offer it under other circumstances, right? Currently it's an unreachable code that confuses the reader. YAGNI.

it's quite fun to so YAGNI reference in a context of 1 extra code line.

any way, SASL mechanism filtering is not mandatory, and it simply was added later than cyrsasl_external module. so this code was triggered in initial implementation, and it was required to close client's session gracefully

Is it important that filtering was added by this PR? The fact remains, that this code will never be executed unless there is a bug which would lead to client successfully choosing EXTERNAL despite lack of certificate.

arcusfelis · 2018-02-21T20:36:52Z

src/cyrsasl_external.erl

+mech_step(#state{creds = Creds}, <<"">>) ->
+  authorize(Creds);
+mech_step(#state{creds = Creds}, User) ->
+  authorize(mongoose_credentials:set(Creds, requested_name, User)).


comment, why do we need to save requested_name.

see XEP 178. generally SASL code is not responsible to make an auth decision, it up to auth backend(s)

Where is this field used in the code?

it can be used by auth module, but the demo pki auth module that we currently have ignores it, as well as most of the cert fields.

arcusfelis · 2018-02-21T20:38:44Z

src/cyrsasl_external.erl

+                                                             ])}}
+  end.
+
+-spec mech_step(State :: tuple(), ClientIn :: binary()) -> R when


types are too generic.
-type saslext_state() :: #state{} and using saslext_state() everywhere.
ClientIn is probably luser().

arcusfelis · 2018-02-21T20:42:43Z

src/cyrsasl_external.erl

+  {error, <<"not-authorized">>};
+mech_step(#state{creds = Creds}, <<"">>) ->
+  authorize(Creds);
+mech_step(#state{creds = Creds}, User) ->


Indention, 4 spaces everywhere.
btw, there was a code style guide somewhere, but definitely not inside the repo.

the link to code style, please

Every module in MIM has 4-space indention, maybe except for some old ejabberd code nobody really touched since fork (I think some pieces might still be left...).

arcusfelis · 2018-02-21T20:44:55Z

src/cyrsasl_external.erl

+  authorize(mongoose_credentials:set(Creds, requested_name, User)).
+
+authorize(Creds) ->
+  %% auth backend is responsible to add username to Creds.


would be good to mention that auth backend is most likely ejabberd_auth_pki.
Or something, that does not use password.

ejabberd_auth_pki is just a demo auth backend it assumes that cert is ok if it passes validation, there's a plan to use DB backends in the future.

arcusfelis · 2018-02-21T20:45:35Z

src/ejabberd_auth_pki.erl

@@ -0,0 +1,117 @@
+-module(ejabberd_auth_pki).


doc header.
license header.

i would add it with a big pleasure, do we have some doc that describes the format of license/doc header?

Be like @kzemek

%%============================================================================== %% Copyright 2017 Erlang Solutions Ltd. %% %% Licensed under the Apache License, Version 2.0 (the "License"); %% you may not use this file except in compliance with the License. %% You may obtain a copy of the License at %% %% http://www.apache.org/licenses/LICENSE-2.0 %% %% Unless required by applicable law or agreed to in writing, software %% distributed under the License is distributed on an "AS IS" BASIS, %% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. %% See the License for the specific language governing permissions and %% limitations under the License. %%============================================================================== -module(mod_global_distrib_utils). -author('[email protected]').

It's for new files, not based on ejabberd's code.

It's in a private company dev-handbook.
https://github.com/esl/dev-handbook/blob/master/development_handbook.markdown#module-header

We should have subset of it for public use, but we don't have yet.

@arcusfelis

I'm not so sure if it's OK to put Apache license in our new modules, because ejabberd files already have GPL license. I'm not that good with legal stuff but isn't there a chance of some conflict here?

Standard company header (according to the devbook) is an optimal choice I guess.

@fenek We license the whole project under terms covered in the COPYING file. I would interpret files with Apache license header to be additionally available under Apache license. Anyway, correct copyright headers help us see which files still prevent us from moving the whole project from GPL to Apache.

It sounds sane but as far as I understand, we are operating on our assumptions, correct? Not a formal opinion of a lawyer experienced in open source licenses.

Anyway, the truth is that the standard header for dev handbook provides more useful information than the one with Apache license... they can be combined so a header fills the first screen of a source code view. :D

OK, now seriously: I'd prefer the handbook header. And this legal stuff is a mess.

arcusfelis · 2018-02-21T20:47:00Z

src/ejabberd_auth_pki.erl

+%%-callback stop(Host :: ejabberd:lserver()) -> ok.
+stop(_) -> ok.
+
+%%-callback store_type(Host :: ejabberd:lserver()) -> scram | plain | external.


really scram, or not relevant?
maybe external

really, this scram/plain/external store types has nothing common with sasl mechanisms. it's more about the way how we store the password. and we will add certificates support in DB based auth backends later. but that backends must be configured as scram. other way that won't be possible to distinguish between passwords and certificates

arcusfelis · 2018-02-21T20:47:24Z

src/ejabberd_auth_pki.erl

+%%-callback authorize(mongoose_credentials:t()) -> {ok, mongoose_credentials:t()} | {error, any()}.
+authorize(Creds) ->
+  case get_username(Creds) of
+    {ok,UserName} ->


whitespace ,

arcusfelis · 2018-02-21T20:48:46Z

src/ejabberd_auth_pki.erl

+%%-callback store_type(Host :: ejabberd:lserver()) -> scram | plain | external.
+store_type(_) -> scram.
+
+%%-callback set_password( User :: ejabberd:luser(),


we don't need callback comments for functions that do nothing (i.e. just return one value always).

of course we do need them. it is very basic module, but it can be extended for the client's needs.

These comments don't do any harm.

arcusfelis · 2018-02-21T20:51:31Z

src/ejabberd_auth_pki.erl

+%% internal functions
+%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
+get_username(Cred) ->
+  case mongoose_credentials:get(Cred,common_name,no_cert) of


whitespaces, probably indention.
We use style

case X of something -> something end.

please reference to the doc

Personally I don't mind lack of newline when all matches in a case are one-liners.

arcusfelis · 2018-02-21T20:53:10Z

src/ejabberd_c2s.erl

    case cyrsasl:listmech(Server) of
        [] -> [];
        Mechanisms ->
            [#xmlel{name = <<"mechanisms">>,
                    attrs = [{<<"xmlns">>, ?NS_SASL}],
-                    children = [ mechanism(S) || S <- Mechanisms ]}]
+                    children = [ mechanism(M) || M <- Mechanisms, filter_mechanism(M,S) ]}]


arcusfelis · 2018-02-21T21:38:02Z

test.disabled/ejabberd_tests/tests/connect_SUITE.erl

-                                  auth_compression_bind_session,
-                                  auth_bind_compression_session,
-                                  bind_server_generated_resource]}
+    [ {c2s_noproc, [], [reset_stream_noproc,


we don't need to touch the whole block.
unless you want to create functions feature_order_cases(), c2s_noproc_cases(), .....

arcusfelis · 2018-02-21T21:39:49Z

test.disabled/ejabberd_tests/tests/connect_SUITE.erl

@@ -256,7 +269,11 @@ clients_can_connect_with_advertised_ciphers(Config) ->
                         ciphers_working_with_ssl_clients(Config))).

 'clients_can_connect_with_DHE-RSA-AES256-SHA_only'(Config) ->
-    Config1 = [{c2s_port, 5233} | Config],
+    Port = case ?config(tls_module, Config) of
+               just_tls -> 5263; %mim3 secondary_c2s port


arcusfelis · 2018-02-26T09:34:12Z

src/ejabberd_s2s_in.erl

+                    children = [#xmlel{name = <<"mechanism">>,
+                                       children = [#xmlcdata{content = <<"EXTERNAL">>}]}]}];
+        {bad_cert, CertVerifyRes} ->
+            check_sasl_tls_certveify(TLSCertverify, CertVerifyRes);


typo in verify

fenek · 2018-02-26T18:20:32Z

src/cert_utils.erl

+  Val = convert_to_bin(V),
+  case {jid:from_binary(Val), UTF8} of
+    {#jid{luser = <<"">>, lserver = LD, lresource = <<"">>}, true} ->
+      case ejabberd_s2s:domain_utf8_to_ascii(LD) of


I don't like calling utility function from ejabberd_s2s in a module that is not dedicated to S2S logic. I think I could live with some code duplication or even better - moving domain_utf8_to_ascii to some util module.

first version of this module is just cut&paste from ejabberd_s2s_in module, than the code was cleaned up to make it reusable from cyrsasl_external

Add mam_iq:lookup_params/0 type Remove remote calls from mam_iq to itself Log dropped stanzas in parallel stories (configurable in test.config) Reduce concurrency in mam rsm test cases (separate group for complete kind of tests) Use priority=-1 for parallel tests

no function changes. just renaming, improvements in type definitions, etc.

codecov-io · 2018-03-07T17:52:36Z

Codecov Report

Merging #1735 into master will decrease coverage by 0.2%.
The diff coverage is 43.66%.

@@            Coverage Diff             @@
##           master    #1735      +/-   ##
==========================================
- Coverage   74.65%   74.45%   -0.21%     
==========================================
  Files         283      288       +5     
  Lines       26578    26760     +182     
==========================================
+ Hits        19843    19923      +80     
- Misses       6735     6837     +102

Impacted Files	Coverage Δ
src/ejabberd.erl	`31.25% <ø> (ø)`	⬆️
src/cyrsasl_external.erl	`0% <0%> (ø)`
src/cert_utils.erl	`0% <0%> (ø)`
src/ejabberd_auth_pki.erl	`0% <0%> (ø)`
...c/global_distrib/mod_global_distrib_server_sup.erl	`100% <100%> (ø)`	⬆️
src/mam_iq.erl	`88.67% <100%> (ø)`	⬆️
...c/global_distrib/mod_global_distrib_server_mgr.erl	`80% <100%> (ø)`	⬆️
src/mod_mam.erl	`89.39% <100%> (+0.37%)`	⬆️
src/mod_mam_muc.erl	`70.37% <100%> (ø)`	⬆️
src/mod_mam_utils.erl	`87.37% <100%> (+1.36%)`	⬆️
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c501f7...ef1af53. Read the comment docs.

arcusfelis reviewed Feb 19, 2018

View reviewed changes

DenysGonchar added the WIP 🚧 label Feb 19, 2018

arcusfelis reviewed Feb 19, 2018

View reviewed changes

DenysGonchar force-pushed the SASL_EXTERNAL_implementation branch 2 times, most recently from 5d72381 to a350408 Compare February 21, 2018 14:49

arcusfelis reviewed Feb 21, 2018

View reviewed changes

arcusfelis reviewed Feb 26, 2018

View reviewed changes

fenek reviewed Feb 26, 2018

View reviewed changes

DenysGonchar force-pushed the SASL_EXTERNAL_implementation branch 3 times, most recently from 9eede7e to 1b7d0f3 Compare March 7, 2018 08:49

fenek and others added 7 commits March 7, 2018 13:59

Fix a race condition in a call to GD conn manager

b4d2dc2

[skip ci] Add TODO about tests in GD server sup

fc3029b

Bump escalus version to aa77ba506fd46cf2bf9a0ac3639bc2ed6e1a8173 (#1756)

64122ec

added tls wrapper module

2617403

call tcp_to_tls from the socket owner process

e7dbe28

standard Erlang SSL backend

772775d

DenysGonchar force-pushed the SASL_EXTERNAL_implementation branch from 1b7d0f3 to 19af40b Compare March 7, 2018 13:07

DenysGonchar added 7 commits March 7, 2018 14:36

standard Erlang SSL backend testing

37ce0e5

SASL EXTERNAL mechanism implementation

0738dfd

added crlfiles c2s configuration

39a531e

moving certificate parsing logic into the separate module

fe9ceb7

passing cert's fields to auth. module

bf5fad5

dummy PKI auth. module

28c0df9

rework according to review comments

ef1af53

no function changes. just renaming, improvements in type definitions, etc.

DenysGonchar force-pushed the SASL_EXTERNAL_implementation branch from 19af40b to ef1af53 Compare March 7, 2018 13:38

DenysGonchar merged commit ac965ca into master Mar 7, 2018

DenysGonchar deleted the SASL_EXTERNAL_implementation branch March 8, 2018 20:21

DenysGonchar removed the WIP 🚧 label Mar 8, 2018

fenek added this to the 3.0.0 milestone Mar 14, 2018

This was referenced May 4, 2018

MongooseIM 2.2 documentation #1834

Closed

Client side certificate #620

Closed

		Val.


		get_lserver_from_addr(V, UTF8) when is_binary(V); is_list(V) ->


		-behaviour(cyrsasl).

		-record(state, {creds}).

SASL EXTERNAL implementation #1735

SASL EXTERNAL implementation #1735

Conversation

DenysGonchar commented Feb 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcusfelis Feb 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcusfelis Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DenysGonchar Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcusfelis Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzemek Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcusfelis Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

arcusfelis Feb 26, 2018 •

edited

Loading

arcusfelis Feb 21, 2018 •

edited

Loading

DenysGonchar Feb 27, 2018 •

edited

Loading

arcusfelis Feb 21, 2018 •

edited

Loading

kzemek Feb 27, 2018 •

edited

Loading

arcusfelis Feb 21, 2018 •

edited

Loading