Support for self-signed certificates #318

Merged
mmarif merged 32 commits from :self-signed into master 2020-04-03 06:16:05 +00:00
Member

This code is based on https://github.com/ge0rg/MemorizingTrustManager, but i had to change a lot because it is not well maintained.

This code is based on https://github.com/ge0rg/MemorizingTrustManager, but i had to change a lot because it is not well maintained.
6543 added the
Feature
label 2020-03-30 21:27:19 +00:00
6543 added this to the 2.5.0 milestone 2020-03-30 21:27:26 +00:00
mmarif reviewed 2020-03-31 04:53:03 +00:00
Dismissed
@ -483,7 +481,6 @@ public class LoginActivity extends BaseActivity implements View.OnClickListener
if (response.isSuccessful()) {
if (response.code() == 200) {

Reason to remove this blank line?

Reason to remove this blank line?
@ -461,2 +458,3 @@
public void onFailure(@NonNull Call<GiteaVersion> callVersion, @NotNull Throwable throwable) {
Log.e("onFailure-version", throwable.toString());
}

Reason to remove this blank line?

Reason to remove this blank line?
@ -21,6 +21,7 @@ import android.widget.TextView;
import androidx.annotation.NonNull;
import androidx.appcompat.app.AlertDialog;
import com.tooltip.Tooltip;
import org.jetbrains.annotations.NotNull;

Should only import androidx NonNull import androidx.annotation.NonNull; instead

Should only import androidx NonNull `import androidx.annotation.NonNull;` instead
@ -458,3 +458,1 @@
Log.e("onFailure-version", t.toString());
public void onFailure(@NonNull Call<GiteaVersion> callVersion, @NotNull Throwable throwable) {

Use NonNull

Changing t to throwable is not necessary

Use NonNull Changing `t` to throwable is not necessary
@ -21,3 +26,3 @@
public class IssuesService {
public static <S> S createService(Class<S> serviceClass, String instanceURL, Context ctx) {
public static <S> S createService(Class<S> serviceClass, String instanceURL, Context context) {

ctx can be kept as is.

`ctx` can be kept as is.
@ -61,0 +62,4 @@
Retrofit retrofit = builder.build();
return retrofit.create(serviceClass);
} catch(Exception e) {

catch should be in new line

catch should be in new line
@ -61,0 +63,4 @@
Retrofit retrofit = builder.build();
return retrofit.create(serviceClass);
} catch(Exception e) {
e.printStackTrace();

Please use Logger Log.e();

Please use Logger `Log.e();`
@ -21,3 +26,3 @@
public class PullRequestsService {
public static <S> S createService(Class<S> serviceClass, String instanceURL, Context ctx) {
public static <S> S createService(Class<S> serviceClass, String instanceURL, Context context) {

Same comments apply as IssuesService

Same comments apply as IssuesService
@ -54,2 +42,3 @@
sslContext.init(null, new X509TrustManager[]{memorizingTrustManager}, new SecureRandom());
Retrofit retrofit = builder.build();
OkHttpClient okHttpClient = new OkHttpClient.Builder().cache(cache).sslSocketFactory(sslContext.getSocketFactory(), memorizingTrustManager).hostnameVerifier(memorizingTrustManager.wrapHostnameVerifier(HttpsURLConnection.getDefaultHostnameVerifier())).addInterceptor(new Interceptor() {

Please keep the new lines as is and keep //.addInterceptor(logging) also for debugging purpose.

Please keep the new lines as is and keep `//.addInterceptor(logging)` also for debugging purpose.
@ -46,3 +49,1 @@
} else {
request = request.newBuilder().header("Cache-Control", "public, only-if-cached, max-stale=" + 60 * 60 * 24 * 30).build();
}

Please use if statement instead if possible.

Please use if statement instead if possible.
@ -0,0 +2,4 @@
/**
* Author Georg Lukas, modified by anonTree1417
*/

New line after this

New line after this
@ -0,0 +13,4 @@
import android.util.SparseArray;
import android.os.Build;
import android.os.Handler;

Please remove all blank lines between imports

Please remove all blank lines between imports
@ -569,0 +572,4 @@
<string name="mtm_cert_expired">The server certificate is expired.</string>
<string name="mtm_accept_servername">Accept Mismatching Server Name?</string>
<string name="mtm_hostname_mismatch">Server could not authenticate as \&quot;%s\&quot;. The certificate is only valid for:</string>

No blank line required here

No blank line required here
@ -569,0 +575,4 @@
<string name="mtm_connect_anyway">Do you want to connect anyway?</string>
<string name="mtm_cert_details">Certificate details:</string>

Here too

Here too
@ -569,0 +579,4 @@
<string name="mtm_decision_always">Always</string>
<string name="mtm_decision_once">Once</string>
<string name="mtm_decision_abort">Abort</string>

And here too

And here too
Owner

Thanks for your contribution.

I have just commented only for the formatting and things which are redundant and can be removed like change ctx to context OR t to throwable etc.

You can read this wiki page https://gitea.com/gitnex/GitNex/wiki/Code-Standards to have better idea.

We also have code style to format the code(not all) for you, under Code -> Reformat code in AS.

Few other things to take note of are:

I haven't tested the functionality yet. Will do that soon.

Thanks for your contribution. I have just commented only for the formatting and things which are redundant and can be removed like change ctx to context OR t to throwable etc. You can read this wiki page https://gitea.com/gitnex/GitNex/wiki/Code-Standards to have better idea. We also have code style to format the code(not all) for you, under Code -> Reformat code in AS. Few other things to take note of are: - Move ssl package to helpers package - Mention the Author of the code in README file https://gitea.com/gitnex/GitNex/src/branch/master/README.md I haven't tested the functionality yet. Will do that soon.
Member

pleace resolve confilcts

pleace resolve confilcts
Author
Member

@mmarif Should be done with d2190ad189 and e1ccb4e404

@mmarif Should be done with d2190ad18937246508d5746cc988ade14b565d20 and e1ccb4e40472a27082ea1f65951036161a670803
Author
Member

Resolved with 5ee872dc6e

Resolved with 5ee872dc6e871b514f19bb51d01fbde5eb1440cd
opyale changed title from Support for self-signed certificates to WIP: Support for self-signed certificates 2020-03-31 18:52:23 +00:00
Author
Member

I have to work on some improvements.

I have to work on some improvements.
Owner

Ok, please let us know once you are done with all the improvements, fixes, formatting etc.

Ok, please let us know once you are done with all the improvements, fixes, formatting etc.
opyale changed title from WIP: Support for self-signed certificates to Support for self-signed certificates 2020-03-31 20:13:53 +00:00
Author
Member

Everything i wanted to fix (and improve) is now done.

Everything i wanted to fix (and improve) is now done.
Author
Member

Error message when wrong port is given.

Error message when wrong port is given.
Author
Member

Both dialogs, if certificate is self-signed or hostname doesn't match.

Both dialogs, if certificate is self-signed or hostname doesn't match.
opyale changed title from Support for self-signed certificates to WIP: Support for self-signed certificates 2020-04-01 10:27:48 +00:00
opyale changed title from WIP: Support for self-signed certificates to Support for self-signed certificates 2020-04-01 11:27:29 +00:00
Author
Member

Added support for Picasso 0dda996444

Added support for Picasso 0dda996444b2d2c2ff965a43250aa7964ea2014d
Owner

Hi @anonTree1417

We will be removing extra services from clients. Saw that you added a new one. I would suggest to use RetrofitClient instead. #309

Also please test this PR against all cases like having ssl, http etc.

Out of curiosity, what scenario you have to use self signed certificate?

Hi @anonTree1417 We will be removing extra services from clients. Saw that you added a new one. I would suggest to use RetrofitClient instead. #309 Also please test this PR against all cases like having ssl, http etc. Out of curiosity, what scenario you have to use self signed certificate?
Author
Member

@mmarif

I would suggest to use RetrofitClient instead.

I dont think this is possible with Picasso, because RetrofitClient seems to be used to access plain(-text) data only, not images.

Also please test this PR against all cases like having ssl, http etc.

I have tested a lot already as you can already see in my screenshots.

Out of curiosity, what scenario you have to use self signed certificate?

I am using it for my personal server. I cant sign any certificates with Let'sEncrypt for example, because i dont have any domain (access via dynamic ip).

@mmarif > I would suggest to use RetrofitClient instead. I dont think this is possible with Picasso, because RetrofitClient seems to be used to access plain(-text) data only, not images. > Also please test this PR against all cases like having ssl, http etc. I have tested a lot already as you can already see in my screenshots. > Out of curiosity, what scenario you have to use self signed certificate? I am using it for my personal server. I cant sign any certificates with Let'sEncrypt for example, because i dont have any domain (access via dynamic ip).
opyale reviewed 2020-04-01 13:16:32 +00:00
Dismissed
@ -0,0 +36,4 @@
.sslSocketFactory(sslContext.getSocketFactory(), memorizingTrustManager)
.hostnameVerifier(memorizingTrustManager.wrapHostnameVerifier(HttpsURLConnection.getDefaultHostnameVerifier()));
builder.downloader(new OkHttp3Downloader(okHttpClient.build()));
Author
Member

Picasso needs a downloader (in this case OkHttp3Downloader in conjunction with a OkHttpClient).

Picasso needs a downloader (in this case OkHttp3Downloader in conjunction with a OkHttpClient).
Author
Member

Also please test this PR against all cases like having ssl, http etc.

All these changes do also only apply if a self-signed certificate is used.
If there were any major bugs, they would most likely not occur when using servers with officially signed certificates.

But as already mentioned, i have tested everything so far.

> Also please test this PR against all cases like having ssl, http etc. All these changes do also only apply if a self-signed certificate is used. If there were any major bugs, they would most likely not occur when using servers with officially signed certificates. But as already mentioned, i have tested everything so far.
Owner

Ok, cool then.

I will test later if you are done with it.

Ok, cool then. I will test later if you are done with it.
Author
Member

@mmarif I'm already done.

@mmarif I'm already done.
Owner

I have just tested this PR. Before I comment on things, I have some changes to push to your fork. Can you add me and @6543 to your forked repo.

I have just tested this PR. Before I comment on things, I have some changes to push to your fork. Can you add me and @6543 to your forked repo.
Author
Member

But please make a new branch of self-signed. Then we can merge those two.

But please make a new branch of self-signed. Then we can merge those two.
Owner

But please make a new branch of self-signed. Then we can merge those two.

It should not be done that way. Normally it should be pushed to current branch, which then will automatically update upstream with new commit.

But anyway I have granted your wish. New branch is self-signed2.

> But please make a new branch of self-signed. Then we can merge those two. It should not be done that way. Normally it should be pushed to current branch, which then will automatically update upstream with new commit. But anyway I have granted your wish. New branch is self-signed2.
Author
Member

Thanks.

Thanks.
Owner

Now comes to the suggestions/ideas etc.

try { // try-catch can be problematic here

What could be the problem here and what does it mean?

Log.e("PicassoService", Objects.requireNonNull(uri.toString())); // important!!
Log.e("PicassoService", exception.toString());

What is this Log.e(// important!!) for?

implementation "org.conscrypt:conscrypt-android:2.2.1"

I have commented out this line and the app does work. What does this library do?
Please check the app without and see if it works. If so remove this library afterwards.

If I am disconnected, the cache does not work and app does not ask for the cert(when choose ONCE).

This could be just me, but I find ONCE annoying and unncessary. Almost every screen few API calls, the dialog keep poping up. To me Abort and Trust buttons are enough.
What you think @6543

This is important one, does the cert saved inside app local space? In that case how to remove the cert if I don't need it anymore?

Now comes to the suggestions/ideas etc. > try { // try-catch can be problematic here What could be the problem here and what does it mean? ``` Log.e("PicassoService", Objects.requireNonNull(uri.toString())); // important!! Log.e("PicassoService", exception.toString()); ``` What is this Log.e(// important!!) for? > implementation "org.conscrypt:conscrypt-android:2.2.1" I have commented out this line and the app does work. What does this library do? Please check the app without and see if it works. If so remove this library afterwards. If I am disconnected, the cache does not work and app does not ask for the cert(when choose ONCE). This could be just me, but I find `ONCE` annoying and unncessary. Almost every screen few API calls, the dialog keep poping up. To me Abort and Trust buttons are enough. What you think @6543 This is important one, does the cert saved inside app local space? In that case how to remove the cert if I don't need it anymore?
Author
Member

What could be the problem here and what does it mean?

When an exception is thrown the object of Retrofit could be not fully initialized.

What is this Log.e(// important!!) for?

This line is very important for debugging. It prints on which URL the error occurred. This can be useful if the server was badly configured and the configured host is different from your current host.

implementation “org.conscrypt:conscrypt-android:2.2.1”

#316 (comment)

This could be just me, but I find ONCE annoying and unncessary. Almost every screen few API calls, the dialog keep poping up. To me Abort and Trust buttons are enough.

This behaviour is related to how the system works. But i can remove that. I shouldnt matter for the average user.

> What could be the problem here and what does it mean? When an exception is thrown the object of Retrofit could be not fully initialized. > What is this Log.e(// important!!) for? This line is very important for debugging. It prints on which URL the error occurred. This can be useful if the server was badly configured and the configured host is different from your current host. > implementation “org.conscrypt:conscrypt-android:2.2.1” https://gitea.com/gitnex/GitNex/issues/316#issuecomment-109604 > This could be just me, but I find ONCE annoying and unncessary. Almost every screen few API calls, the dialog keep poping up. To me Abort and Trust buttons are enough. This behaviour is related to how the system works. But i can remove that. I shouldnt matter for the average user.
Author
Member

and the configured host is different from your current host.

eg.:

CURRENT_HOST -> gitea.com
CONFIGURED_HOST -> 123.123.123.123
> and the configured host is different from your current host. eg.: ``` CURRENT_HOST -> gitea.com CONFIGURED_HOST -> 123.123.123.123 ```
Owner

When an exception is thrown the object of Retrofit could be not fully initialized.

Thats fine then.

This line is very important for debugging. It prints on which URL the error occurred. This can be useful if the server was badly configured and the configured host is different from your current host.

If it's only for debugging purpose, then for actual code it should be commented out like how logging intercepter is commented.

implementation “org.conscrypt:conscrypt-android:2.2.1”

Does disabling this library gives you any error? Becuase it does not for me.

This behaviour is related to how the system works. But i can remove that. I shouldnt matter for the average user.

Yes, Abort and Trust are enough.

Please see my changes and let me know if you have any question.

> When an exception is thrown the object of Retrofit could be not fully initialized. Thats fine then. > This line is very important for debugging. It prints on which URL the error occurred. This can be useful if the server was badly configured and the configured host is different from your current host. If it's only for debugging purpose, then for actual code it should be commented out like how logging intercepter is commented. > implementation “org.conscrypt:conscrypt-android:2.2.1” Does disabling this library gives you any error? Becuase it does not for me. > This behaviour is related to how the system works. But i can remove that. I shouldnt matter for the average user. Yes, Abort and Trust are enough. Please see my changes and let me know if you have any question.
Author
Member

Please see my changes and let me know if you have any question.

Your changes are fine. I will merge them.

Yes, Abort and Trust are enough.

If it’s only for debugging purpose, then for actual code it should be commented out like how logging intercepter is commented.

Ok.

Does disabling this library gives you any error? Becuase it does not for me.

I will test that again.

> Please see my changes and let me know if you have any question. Your changes are fine. I will merge them. > Yes, Abort and Trust are enough. > If it’s only for debugging purpose, then for actual code it should be commented out like how logging intercepter is commented. Ok. > Does disabling this library gives you any error? Becuase it does not for me. I will test that again.
Author
Member

@mmarf Ok, i have implemented your wishes. The only thing i am struggling with at the moment is the library. Everything works for me, even if the library is missing, BUT if its missing, exceptions get thrown nevertheless. Could you check if your stacktrace shows similar signs in that case?

@mmarf Ok, i have implemented your wishes. The only thing i am struggling with at the moment is the library. Everything works for me, even if the library is missing, BUT if its missing, exceptions get thrown nevertheless. Could you check if your stacktrace shows similar signs in that case?
Owner

It does not throw any error. Aside that it does add 5MB to the package size of the APK though.

I would suggest to remove it.

Also comment out the 2nd Log.e.

Log.e("PicassoService", exception.toString());

Once done, I will test again.

It does not throw any error. Aside that it does add 5MB to the package size of the APK though. I would suggest to remove it. Also comment out the 2nd Log.e. `Log.e("PicassoService", exception.toString());` Once done, I will test again.
Author
Member

Ok, done.

Ok, done.
6543 reviewed 2020-04-01 22:57:56 +00:00
Dismissed
@ -178,9 +178,9 @@ public class IssueCommentsAdapter extends RecyclerView.Adapter<IssueCommentsAdap
}
if (currentItem.getUser().getAvatar_url() != null) {

@mmarif this need a refactor! if () do x else do same ?!?

@mmarif this need a refactor! if () do x else do same ?!?
Author
Member

This issue is not related to this pull request.

This issue is not related to this pull request.
6543 reviewed 2020-04-01 23:07:57 +00:00
Dismissed
6543 left a comment
Member

looked throu the code ...

looked throu the code ...
@ -182,9 +182,9 @@ public class IssuesAdapter extends RecyclerView.Adapter<RecyclerView.ViewHolder>
}
if (issuesModel.getUser().getAvatar_url() != null) {

same useless if statement here

same useless if statement here
Author
Member

This issue is not related to this pull request.

This issue is not related to this pull request.

I know butit would fit in ...

I know butit would fit in ...

You can send an extra PR if you like to divide stuff

You can send an extra PR if you like to divide stuff
Author
Member

I dont know whether mixing these different things up would really fit.

I dont know whether mixing these different things up would really fit.
Author
Member

Maybe a thing for another pull reuqest?

Maybe a thing for another pull reuqest?
Member

@anonTree1417 you could add a refacotr to IssueCommentsAdapter.java and IssuesAdapter.java both have a if function witch executes the same code

if ( true) { A(); } else { A(); } == A()

@anonTree1417 you could add a refacotr to `IssueCommentsAdapter.java` and `IssuesAdapter.java` both have a if function witch executes the same code `if ( true) { A(); } else { A(); }` == `A()`
Author
Member

@6543 This issue is not related to this pull request. (again)

@6543 This issue is not related to this pull request. (again)
Member

It would result in a merge confilct if I create a pr for it ...

It would result in a merge confilct if I create a pr for it ...
Author
Member

Why? I can resolve those...

Why? I can resolve those...
Author
Member

Just create another pull request and i will resolve any related conflicts

Just create another pull request and i will resolve any related conflicts
Member

I found nothing related to your code so this pr should be fine - but didn't test it against local-noSSL; local-SSL, global-validSSL

I found nothing related to your code so this pr should be fine - but didn't test it against local-noSSL; local-SSL, global-validSSL
Member

and one function I miss: clear manualy-acepted certs in app-settings

EDIT: can you add a smal config menue to clear the cache or eafen show whats stored i int to the app-congis?

and one function I miss: clear manualy-acepted certs in app-settings EDIT: can you add a smal config menue to clear the cache or eafen show whats stored i int to the app-congis?
Author
Member

@6543 Hope, youre happy now.
abdfe8d9d1 b9eee16294

@6543 Hope, youre happy now. abdfe8d9d1d46f0c5216a941c9061eeee8dd874a b9eee16294fc383403d57b999586736b45cd0297
Owner

@anonTree1417 great work.

I will test later with the new changes you added. If anything I will push to self-sign2 branch.

@anonTree1417 great work. I will test later with the new changes you added. If anything I will push to self-sign2 branch.
Author
Member

Im resolving all conflicts now.. Please pull all changes to self-signed2 branch.

Im resolving all conflicts now.. Please pull all changes to self-signed2 branch.
Member

@anonTree1417 tested this is comming to an end :) and yes the option for the config menue is great!!

only one bug I found: "you have to hit login 3 times to pass"

video: https://cloud.obermui.de/s/ajLaBXPRkzMacLZ

@anonTree1417 tested this is comming to an end :) and yes the option for the config menue is great!! only one bug I found: "you have to hit login 3 times to pass" video: https://cloud.obermui.de/s/ajLaBXPRkzMacLZ
Owner

@anonTree1417 I have just pushed few changes. Please merge for upstream. The rest is all good.

Cache is still not working but maybe that will be for another day.

Just noticed the same thing as @6543 mentioned. If this can be fixed, we are good for merging this.

@anonTree1417 I have just pushed few changes. Please merge for upstream. The rest is all good. Cache is still not working but maybe that will be for another day. **Just noticed the same thing as @6543 mentioned. If this can be fixed, we are good for merging this.**
Author
Member

only one bug I found: “you have to hit login 3 times to pass”

Should be fixed with 59e8f6f94a

Cache is still not working but maybe that will be for another day.

What kind of cache? (Picasso, Retrofit)
Is this related to my changes?

> only one bug I found: “you have to hit login 3 times to pass” Should be fixed with 59e8f6f94a9206315f158baf7ca6a1eb42fbc5a6 > Cache is still not working but maybe that will be for another day. What kind of cache? (Picasso, Retrofit) Is this related to my changes?
Author
Member

In fact, caching of picasso does not work. Everything else should do.

In fact, caching of picasso does not work. Everything else should do.
Owner

Retrofit.

If you off internet whatever have you browsed while on internet you are able to browse them.

This from my testing does not work here.

Retrofit. If you off internet whatever have you browsed while on internet you are able to browse them. This from my testing does not work here.
Author
Member

If you off internet whatever have you browsed while on internet you are able to browse them.

Why is the cache not working then?

> If you off internet whatever have you browsed while on internet you are able to browse them. Why is the cache not working then?
Author
Member

In fact, caching of picasso does not work. Everything else should do.

It does work. Didnt think of picasso's own, indipendent cache.

> In fact, caching of picasso does not work. Everything else should do. It does work. Didnt think of picasso's own, indipendent cache.
Owner

I have uploaded a sample video to discord develop chat room.

It does not work only with self signed certs logins.

I have uploaded a sample video to discord develop chat room. It does not work only with self signed certs logins.
Author
Member

@mmarif

E/IssuesListFragment -: java.net.ConnectException: Failed to connect to /{hostname}:{port}
E/onFailure: 504
E/closedIssuesListFragment -: 504

Errors thrown with self-signed cert and when offline.

@mmarif ``` E/IssuesListFragment -: java.net.ConnectException: Failed to connect to /{hostname}:{port} E/onFailure: 504 E/closedIssuesListFragment -: 504 ``` Errors thrown with self-signed cert and when offline.
Author
Member

Additional errors

I/PullRequestsListFragment -: 504
I/http: 504
Additional errors ``` I/PullRequestsListFragment -: 504 I/http: 504 ```
Author
Member

HTTP 504 sounds like a gateway timeout

HTTP 504 sounds like a gateway timeout
Author
Member
https://stackoverflow.com/questions/42927216/retrofitokhttp-http-504-unsatisfiable-request-only-if-cached
6543 approved these changes 2020-04-02 22:34:15 +00:00
Dismissed
Author
Member

:) Thank you @6543 for reviewing.

:) Thank you @6543 for reviewing.
Member

@mmarif cache works fine (on vallid ssl or http mode), only avatar pictures are not cached <- witch is also on master the case (#344)

And if you are using ssl with self generated cert it wont cache ... cant find the reason jet but this should not block this pull

@mmarif cache works fine (on vallid ssl or http mode), only avatar pictures are not cached <- witch is also on master the case (#344) And if you are using ssl with self generated cert it wont cache ... cant find the reason jet but this should not block this pull
Author
Member

only avatar pictures are not cached

I will fix that.

> only avatar pictures are not cached I will fix that.
6543 added a new dependency 2020-04-02 23:49:24 +00:00
mmarif approved these changes 2020-04-03 06:15:31 +00:00
Dismissed
mmarif closed this pull request 2020-04-03 06:16:05 +00:00
Owner

Thank you @anonTree1417 for the work on this.

Thank you @anonTree1417 for the work on this.
This repo is archived. You cannot comment on pull requests.
No reviewers
No Milestone
No Assignees
3 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Reference: gitnex/GitNex#318
No description provided.